Skip to content
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

[BUG] Rework how we handle feature test macros #195

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

miscco
Copy link
Collaborator

@miscco miscco commented Jul 11, 2023

Previously, we would define __cpp_lib_meow which interferes with the host standard library.

Worst case we might actually break user code, because we define some macros earlier than the host library would.

To alleviate this and also let a user check what we actually implement we rename our feature test macros to __cccl_lib_meow

We still keep the naming convention of the standard and try to define the standard macros when applicable.

In addition to renaming we also fix some macros that should not be defined and fix some values of others

Fixes #189

@miscco miscco added libcu++ For all items related to libcu++ bug Something isn't working right. labels Jul 11, 2023
@miscco miscco requested review from wmaxey and griwes July 11, 2023 15:24
@miscco miscco changed the title Rework how we handle feature test macros [BUG] Rework how we handle feature test macros Jul 11, 2023
@miscco miscco self-assigned this Jul 11, 2023
@griwes
Copy link
Collaborator

griwes commented Jul 14, 2023

I think this approach is sound, but I don't know how I feel about the prefix you went for. I think I'd prefer something more akin to __cuda_cpp or just __cuda, rather than __cccl. (__cuda is a bit tempting, given how we've gone with __cuda_std__ for the main macro proclaiming we're in libcu++ mode.) I don't know if "cccl" actually exists anywhere in the code right now, and I don't know if we want it to start existing.

@miscco
Copy link
Collaborator Author

miscco commented Jul 14, 2023

I think this approach is sound, but I don't know how I feel about the prefix you went for. I think I'd prefer something more akin to __cuda_cpp or just __cuda, rather than __cccl. (__cuda is a bit tempting, given how we've gone with __cuda_std__ for the main macro proclaiming we're in libcu++ mode.) I don't know if "cccl" actually exists anywhere in the code right now, and I don't know if we want it to start existing.

We actually discussed this in the review hour. Originally I had __cuda_lib_meow but that was rejected because it is not really cuda and lacks relation to C++. After a few suggestions we happily settled on __cccl_lib_meow because that clearly signifies our libraries and will potentially allow us to use feature test macros also for cub / thrust

@griwes
Copy link
Collaborator

griwes commented Jul 14, 2023

Let me sleep on that, then; I don't know if that last point you made makes me like this prefix less or more.

@miscco
Copy link
Collaborator Author

miscco commented Jul 14, 2023

Let me sleep on that, then; I don't know if that last point you made makes me like this prefix less or more.

I think we would need to bring this up monday in the code review meeting

@miscco miscco merged commit 2e06e0b into NVIDIA:main Jul 18, 2023
368 checks passed
@miscco miscco deleted the feature_tests branch July 18, 2023 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working right. libcu++ For all items related to libcu++
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG]: Do not override host library feature test macros
3 participants