-
Notifications
You must be signed in to change notification settings - Fork 166
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
unify implementation of [[deprecated]]
attribute
#2934
base: main
Are you sure you want to change the base?
Conversation
/ok to test |
// prefer compiler specific way of defining deprecated because of compiler bugs in old GCC and Clang | ||
#if _CCCL_COMPILER(MSVC) | ||
# define _CCCL_DEPRECATED __declspec(deprecated) | ||
# define _CCCL_DEPRECATED_BECAUSE(_MSG) __declspec(deprecated(_MSG)) | ||
#elif _CCCL_COMPILER(GCC) || _CCCL_COMPILER(CLANG) || _CCCL_COMPILER(NVHPC) || _CCCL_COMPILER(ICC) | ||
# define _CCCL_DEPRECATED __attribute__((__deprecated__)) | ||
# define _CCCL_DEPRECATED_BECAUSE(_MSG) __attribute__((__deprecated__(_MSG))) | ||
#elif _CCCL_STD_VER >= 2014 | ||
# define _CCCL_DEPRECATED [[deprecated]] | ||
# define _CCCL_DEPRECATED_BECAUSE(_MSG) [[deprecated(_MSG)]] |
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.
Important: the change of order was pushed after my approval and worsens the status-quo. Was this required because of compilation failures?
I want to get rid of vendor specific attributes as soon as possible, so we should know what compiler is blocking us here.
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.
Appologies for the push after approval.
Yes, the compilation fails for older GCC and Clang when combining multiple attributes. See the example https://godbolt.org/z/sarjePhPM
I will find out the exact versions and set the vendor's specific attributes only for them. Is this an acceptable solution?
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.
If this affects a lot of compilers, I am also fine with taking your suggestion of prefering vendor attributes. But we should know when we can kick those out, so please add a comment about that. Thx!
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.
Appologies for the push after approval.
We don't have a rule that forbids anybody doing that, and we actually frequently have to push fixes discovered by the CI after approval. So nothing wrong here :) It just happened to change how I feel about the PR.
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.
I've come up with a better solution. First, the macro is defined to vendor's specific attributes. However, if C++14 is available I use [[deprecated]]
instead unless a defected version of GCC or Clang is used.
Hopefully this is an acceptable and functional version :)
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.
Thank you for doing the research!
LGTM
Given that we already need to run the full CI matrix, could you also replace all internal uses of the old macros with the new one? |
Which ones do you mean? If you mean use of |
Yeah I meant the different libraries and we should define a common mechanism to suppress the deprecation warning |
/ok to test |
Looks like we need to ensure that |
🟨 CI finished in 2h 20m: Pass: 93%/402 | Total: 7d 15h | Avg: 27m 21s | Max: 1h 31m | Hits: 11%/19433
|
Project | |
---|---|
+/- | CCCL Infrastructure |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
CUDA Experimental | |
python | |
CCCL C Parallel Library | |
Catch2Helper |
Modifications in project or dependencies?
Project | |
---|---|
+/- | CCCL Infrastructure |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | python |
+/- | CCCL C Parallel Library |
+/- | Catch2Helper |
🏃 Runner counts (total jobs: 402)
# | Runner |
---|---|
327 | linux-amd64-cpu16 |
32 | linux-amd64-gpu-v100-latest-1 |
28 | linux-arm64-cpu16 |
15 | windows-amd64-cpu16 |
It got a bit more complicated again :/ Current changes:
After discussion with @miscco, we've decided to leave rework of suppressing cccl's deprecated suppression macros to a separate PR. |
pre-commit.ci autofix |
This PR moves implementation of
_CCCL_DEPRECATED
and_CCCL_DEPRECATED_BECAUSE
to CCCL.