-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid including yvals.h
when the compiler is not MSVC.
#2545
Conversation
🟩 CI finished in 1h 02m: Pass: 100%/366 | Total: 1d 14h | Avg: 6m 14s | Max: 58m 08s | Hits: 99%/27865
|
Project | |
---|---|
CCCL Infrastructure | |
+/- | libcu++ |
CUB | |
Thrust | |
CUDA Experimental | |
pycuda | |
CCCL C Parallel Library |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | pycuda |
+/- | CCCL C Parallel Library |
🏃 Runner counts (total jobs: 366)
# | Runner |
---|---|
298 | linux-amd64-cpu16 |
28 | linux-arm64-cpu16 |
25 | linux-amd64-gpu-v100-latest-1 |
15 | windows-amd64-cpu16 |
@@ -62,7 +62,7 @@ | |||
//! _CCCL_ASSERT_IMPL_HOST should never be used directly | |||
#if defined(_CCCL_COMPILER_NVRTC) // There is no host standard library in nvrtc | |||
# define _CCCL_ASSERT_IMPL_HOST(expression, message) ((void) 0) | |||
#elif __has_include(<yvals_core.h>) // MSVC uses _STL_VERIFY from <yvals.h> | |||
#elif __has_include(<yvals_core.h>) && defined(_CCCL_COMPILER_MSVC) // MSVC uses _STL_VERIFY from <yvals.h> | |||
# include <yvals.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The __has_include
is checking for a different header file, <yvals_core.h>
, than the header that is actually included, <yvals.h.>
. That looks like a bug that should be fixed, even if it wasn't what triggered the NVBug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that was me changing the include
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STL has both headers, while clang has only the one, @miscco do we need to check for yvals_core.h for Clang's benefit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we should only check for the proper MSVC header and I dont mind also checking for MSVC
🟩 CI finished in 1h 22m: Pass: 100%/366 | Total: 1d 12h | Avg: 5m 58s | Max: 42m 38s | Hits: 99%/27865
|
Project | |
---|---|
CCCL Infrastructure | |
+/- | libcu++ |
CUB | |
Thrust | |
CUDA Experimental | |
pycuda | |
CCCL C Parallel Library |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | pycuda |
+/- | CCCL C Parallel Library |
🏃 Runner counts (total jobs: 366)
# | Runner |
---|---|
298 | linux-amd64-cpu16 |
28 | linux-arm64-cpu16 |
25 | linux-amd64-gpu-v100-latest-1 |
15 | windows-amd64-cpu16 |
* Only include yvals when the compiler is MSVC * Use the right check --------- Co-authored-by: Michael Schellenberger Costa <[email protected]>
Description
closes nvbug/4902968
Checklist