[libcu++] Implement std::constant_wrapper#9046
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughimportant: Adds cuda::std::constant_wrapper (implementation, feature gates, public exposure) and a comprehensive test suite exercising construction, call, subscript, operators, pseudo-mutators, and related feature-detection macros. Changesimportant: Constant Wrapper Feature
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 651d9cca-a982-4876-9fe3-8e0bed904d0a
📒 Files selected for processing (22)
libcudacxx/include/cuda/std/__cccl/dialect.hlibcudacxx/include/cuda/std/__utility/constant_wrapper.hlibcudacxx/include/cuda/std/utilitylibcudacxx/include/cuda/std/versionlibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/adl.compile.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/assign.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/binary_ops.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/call.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/comma.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/comp.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/convert.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/ctad.compile.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/cw.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/cw_fixed.array.ctor.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/cw_fixed.ctor.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/general.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/helpers.hlibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/mem_ptr.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/pseudo_mutators.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/subscript.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/types.compile.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/unary_ops.pass.cpp
| # include <cuda/std/__cstddef/types.h> | ||
| # include <cuda/std/__functional/invoke.h> | ||
| # include <cuda/std/__type_traits/is_constructible.h> | ||
| # include <cuda/std/__type_traits/remove_cvref.h> | ||
| # include <cuda/std/__utility/declval.h> | ||
| # include <cuda/std/__utility/forward.h> | ||
| # include <cuda/std/__utility/integer_sequence.h> | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
suggestion: Add a direct include for is_invocable_v usage.
is_invocable_v is used but its defining header is not included directly; please add the precise type-traits header to avoid transitive-include fragility.
As per coding guidelines: “include only directly-required headers (no transitive includes) with the most precise internal header paths.”
Also applies to: 296-306
davebayer
force-pushed
the
libcuxx_constant_wrapper
branch
from
6f8f1e9 to
4518f37
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7e0e3205-d62f-456c-a899-1eca2f817c7d
📒 Files selected for processing (22)
libcudacxx/include/cuda/std/__cccl/dialect.hlibcudacxx/include/cuda/std/__utility/constant_wrapper.hlibcudacxx/include/cuda/std/utilitylibcudacxx/include/cuda/std/versionlibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/adl.compile.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/assign.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/binary_ops.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/call.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/comma.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/comp.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/convert.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/ctad.compile.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/cw.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/cw_fixed.array.ctor.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/cw_fixed.ctor.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/general.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/helpers.hlibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/mem_ptr.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/pseudo_mutators.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/subscript.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/types.compile.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/unary_ops.pass.cpp
✅ Files skipped from review due to trivial changes (1)
- libcudacxx/include/cuda/std/utility
🚧 Files skipped from review as they are similar to previous changes (3)
- libcudacxx/include/cuda/std/version
- libcudacxx/test/libcudacxx/std/utilities/const.wrap.class/helpers.h
- libcudacxx/include/cuda/std/__cccl/dialect.h
davebayer
force-pushed
the
libcuxx_constant_wrapper
branch
from
4518f37 to
2346681
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libcudacxx/include/cuda/std/__utility/constant_wrapper.h (1)
81-82: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winimportant: make
cwaninline constexprvariable template to satisfy header ODR/guideline expectations.As per coding guidelines: “all namespace/global
constexprvariables must beinline.”
🧹 Nitpick comments (1)
libcudacxx/test/libcudacxx/std/utilities/const.wrap.class/types.compile.pass.cpp (1)
46-50: ⚡ Quick winsuggestion: Line 47 disables the only direct string-literal value-content check, so this file currently validates shape but not payload. Add a temporary compile-time fallback (for example, per-index
static_assertchecks onvalue[0..4]) untilranges::equalis usable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1f32351b-47cf-48c0-9a61-2e03bb1fb3f4
📒 Files selected for processing (22)
libcudacxx/include/cuda/std/__cccl/dialect.hlibcudacxx/include/cuda/std/__utility/constant_wrapper.hlibcudacxx/include/cuda/std/utilitylibcudacxx/include/cuda/std/versionlibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/adl.compile.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/assign.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/binary_ops.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/call.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/comma.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/comp.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/convert.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/ctad.compile.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/cw.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/cw_fixed.array.ctor.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/cw_fixed.ctor.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/general.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/helpers.hlibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/mem_ptr.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/pseudo_mutators.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/subscript.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/types.compile.pass.cpplibcudacxx/test/libcudacxx/std/utilities/const.wrap.class/unary_ops.pass.cpp
✅ Files skipped from review due to trivial changes (2)
- libcudacxx/include/cuda/std/utility
- libcudacxx/include/cuda/std/version
🚧 Files skipped from review as they are similar to previous changes (2)
- libcudacxx/test/libcudacxx/std/utilities/const.wrap.class/helpers.h
- libcudacxx/include/cuda/std/__cccl/dialect.h
| TEST_FUNC void final_phase(auto gathered, auto available) | ||
| { | ||
| if constexpr (gathered == available) | ||
| { | ||
| profit(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C++20, is if constexpr (x == y)valid insidevoid f(auto x, auto y)whenxandy are ordinary function parameters?
💡 Result:
In C++20, the function void f(auto x, auto y) is a function template (using the abbreviated function template syntax) [1]. Inside this function, if constexpr (x == y) is valid only if the expression x == y is a constant expression [2]. Because x and y are function parameters, they are not constant expressions at compile time, even though the function is a template [3][2]. if constexpr requires its condition to be a constant expression that can be evaluated at compile time [2]. Since the values of x and y are only known at runtime, x == y cannot be evaluated at compile time, and therefore if constexpr (x == y) will result in a compilation error [3][2]. If you need to perform conditional logic based on the values of function parameters, you must use a standard if statement [2]. If you need to perform conditional logic based on the types of x and y at compile time, you can use if constexpr with type traits, such as if constexpr (std::is_same_v<decltype(x), decltype(y)>) [1].
Citations:
- 1: https://www.bfilipek.com/2018/03/ifconstexpr/
- 2: https://stackoverflow.com/questions/63273765/conditionally-constexpr-expressions-with-if-constexpr
- 3: https://stackoverflow.com/questions/63046397/why-does-this-usage-of-c17-if-constexpr-fail
🏁 Script executed:
cat -n libcudacxx/test/libcudacxx/std/utilities/const.wrap.class/general.pass.cpp | sed -n '40,55p'Repository: NVIDIA/cccl
Length of output: 491
critical: if constexpr (gathered == available) is ill-formed because function parameters are not constant expressions. The condition in if constexpr must be evaluable at compile time. Use a regular if statement, or if you need compile-time branching based on types, use if constexpr with type traits like std::is_same_v<decltype(gathered), decltype(available)>.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7ed90de6-f814-4373-ac6a-905fe28dc678
📒 Files selected for processing (1)
libcudacxx/test/libcudacxx/std/utilities/const.wrap.class/kernel_parameter.pass.cpp
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // a host member cannot be directly read in a __device__/__global__ function | ||
| _CCCL_BEGIN_NV_DIAG_SUPPRESS(342, 20094) | ||
|
|
||
| // todo: Remove this once #9019 is merged. |
There was a problem hiding this comment.
| // todo: Remove this once #9019 is merged. | |
| // TODO(dbayer): Remove this once #9019 is merged. |
Note the name is not an assignment of the TODO, it is so that future devs know who to ask about this for additional context when fixing in the future
Because the This is definitely not the first PR regarding this feature |
davebayer
force-pushed
the
libcuxx_constant_wrapper
branch
2 times, most recently
from
7ae24b1 to
7fb0b88
Compare
davebayer
force-pushed
the
libcuxx_constant_wrapper
branch
from
7fb0b88 to
e04c6f5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🥳 CI Workflow Results🟩 Finished in 1h 11m: Pass: 100%/116 | Total: 1d 06h | Max: 49m 33s | Hits: 99%/333961See results here. |
2 participants
This PR implements
std::constant_wrapperincuda::std::namespace backported to C++20. The implementation is not that complicated but it almost completely breaks nvcc and older compilers.Fixes #9109.