[libcu++] Improve __constant_sequence#9280
Conversation
The current is_sequence definition is thoroughly broken. An iterator is not a sequence and neither is a pointer. The main problem here is that this precludes passing along iterators / pointers as values which is a reasonable use case. It also completely deviates from any standard handling in C++. Rather than reinventing the wheel in a bad way use standard terminology to determine what is a range and what not. A user can always wrap iterators and pointers into views and spans, so there is not need for the current definition
We should not allow empty sequences for `__constant_sequence` We also do not need all the wrappers
OverviewThis PR improves the Key ChangesArgument Wrapper Sequence Detection (
|
| Layer / File(s) | Summary |
|---|---|
Sequence Detection Rework libcudacxx/include/cuda/__argument/argument.h |
__is_sequence_v now treats types as sequences if they are arrays or satisfy std::ranges::range. __constant_sequence enforces non-empty sequences via ranges::__empty. __immediate_sequence and constant wrapper traits use the updated predicate. |
Constant Bound Computation Inlining libcudacxx/include/cuda/__argument/argument.h |
Removed __constant_compute_* helpers. Bounds for __constant use direct _Value access; bounds for __constant_sequence use min_element/max_element. Free functions __lowest_ and __highest_ updated to match. |
Bounds Validation Utilities libcudacxx/include/cuda/__argument/argument_bounds.h |
Added remove_cvref and cmp includes. Updated bounds-type predicates to strip cv and reference qualifiers. Introduced internal template utilities for range-checked casts, static endpoint checks, effective bound computation, and intersection/value validation. |
Test Updates for Sequence Detection libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp, dynamic_argument.pass.cpp, static_argument.pass.cpp |
Reworked __is_sequence_v assertions: pointers, iterators, and non-range wrappers now classified as non-sequences. Updated is_single_value for int* from false to true. Tests in dynamic and static argument files now check negated __is_sequence_v instead of __is_single_value_v. |
Test Updates for Bounds Validation libcudacxx/test/libcudacxx/cuda/argument/argument_bounds.pass.cpp |
Replaced __has_bounds_intersection with __valid_argument_bounds for static and runtime bounds intersection checks. |
Suggested labels
libcu++
Suggested reviewers
- gevtushenko
- ericniebler
Warning
There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
🔧 Infer (1.2.0)
libcudacxx/test/libcudacxx/cuda/argument/argument_bounds.pass.cpp
libcudacxx/test/libcudacxx/cuda/argument/argument_bounds.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found
11 | #include <cuda/_argument>
| ^~~~~~~~~~~~~~~~~~
1 error generated.
libcudacxx/test/libcudacxx/cuda/argument/argument_bounds.pass.cpp:74:3-81:3: ERROR translating statement 'CompoundStmt'
Aborting translation of method 'test' in file 'libcudacxx/test/libcudacxx/cuda/argument/argument_bounds.pass.cpp': "Assert_failure src/clang/cAst_utils.ml:249:53"
Uncaught Internal Error: "Assert_failure src/clang/cAst_utils.ml:249:53"
Error backtrace:
Raised at ClangFrontend__CAst_utils.get_decl_from_typ_ptr in file "src/clang/cAst_utils.ml", line 249, characters 53-65
Called from ClangFrontend__CTrans.CTrans_funct.get_destructor_decl_ref in file "src/clang/cTrans.ml", line 658, characters 12-59
Called from ClangFrontend__CTrans.CTrans_funct.destructor_calls.(fun) in file "src/clang/cTrans.ml", line 2048, characters 12-69
Called from Base__List.rev_filter_map.
... [truncated 2200 characters] ...
e 13, characters 2-50
Called from ClangFrontend__CTrans.CTrans_funct.instruction_log.(fun) in file "src/clang/cTrans.ml", line 4784, characters 10-1023
Called from ClangFrontend__CTrans.CTrans_funct.instruction in file "src/clang/cTrans.ml" (inlined), line 4765, characters 38-71
Called from ClangFrontend__CTrans.CTrans_funct.exec_with_node_creation in file "src/clang/cTrans.ml" (inlined), line 104, characters 20-38
Called from ClangFrontend__CTrans.CTrans_funct.get_clang_stmt_trans in file "src/clang/cTrans.ml" (inlined), line 5395, characters 4-69
Called from ClangFrontend__CTrans.CTrans_funct.get_custom_stmt_trans in file "src/clang/cTrans.ml", line 5401, characters 8-55
Called from ClangFrontend__CTrans.CTrans_funct.exec_trans_instrs.exec_trans_instrs_rev in file "src/clang/cTrans.ml" (
libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp
libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found
11 | #include <cuda/argument>
| ^~~~~~~~~~~~~~~~~~
1 error generated.
Error: the following clang command did not run successfully:
/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/bin/clang-18
@/tmp/coderabbit-infer/c35fb76fe3c3c38f58e392c98ae7a46d3ad3a2d2-07842b3345a172fc/tmp/clang_command_.tmp.5a42db.txt
++Contents of '/tmp/coderabbit-infer/c35fb76fe3c3c38f58e392c98ae7a46d3ad3a2d2-07842b3345a172fc/tmp/clang_command.tmp.5a42db.txt':
"-cc1" "-load"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../../facebook-clang-plugins/libtooling/build/FacebookClangPlugin.dylib"
"-add-plugin" "BiniouASTExporter" "-plugin-arg-BiniouASTExporter" "-"
"-plugin-arg-BiniouASTExporter" "PREPEND_CURRENT_DIR=1"
"-plugin-arg-BiniouASTExporter" "MAX_STRING_SIZE=65535" "-cc1" "-triple"
"x86_64-unknown-li
... [truncated 1169 characters] ...
al-isystem" "/usr/local/include" "-internal-isystem"
"/usr/lib/gcc/x86_64-linux-gnu/12/../../../../x86_64-linux-gnu/include"
"-internal-externc-isystem" "/usr/include/x86_64-linux-gnu"
"-internal-externc-isystem" "/include" "-internal-externc-isystem"
"/usr/include" "-Wno-ignored-optimization-argument" "-Wno-everything"
"-fdeprecated-macro" "-ferror-limit" "19" "-fgnuc-version=4.2.1"
"-fskip-odr-check-in-gmf" "-fcxx-exceptions" "-fexceptions"
"-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o"
"/tmp/coderabbit-infer/07842b3345a172fc/file.o" "-x" "c++"
"libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp" "-O0"
"-fno-builtin" "-include"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../lib/clang_wrappers/global_defines.h"
"-Wno-everything"
libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp
libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found
11 | #include <cuda/_argument>
| ^~~~~~~~~~~~~~~~~~
1 error generated.
libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp:161:3-166:3: ERROR translating statement 'CompoundStmt'
Aborting translation of method 'test' in file 'libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp': "Assert_failure src/clang/cAst_utils.ml:249:53"
Uncaught Internal Error: "Assert_failure src/clang/cAst_utils.ml:249:53"
Error backtrace:
Raised at ClangFrontend__CAst_utils.get_decl_from_typ_ptr in file "src/clang/cAst_utils.ml", line 249, characters 53-65
Called from ClangFrontend__CTrans.CTrans_funct.get_destructor_decl_ref in file "src/clang/cTrans.ml", line 658, characters 12-59
Called from ClangFrontend__CTrans.CTrans_funct.destructor_calls.(fun) in file "src/clang/cTrans.ml", line 2048, characters 12-69
Called from Base__List.rev_filter
... [truncated 2200 characters] ...
from ClangFrontend__CTrans.CTrans_funct.exec_with_node_creation in file "src/clang/cTrans.ml" (inlined), line 104, characters 20-38
Called from ClangFrontend__CTrans.CTrans_funct.get_clang_stmt_trans in file "src/clang/cTrans.ml" (inlined), line 5395, characters 4-69
Called from ClangFrontend__CTrans.CTrans_funct.get_custom_stmt_trans in file "src/clang/cTrans.ml", line 5401, characters 8-55
Called from ClangFrontend__CTrans.CTrans_funct.exec_trans_instrs.exec_trans_instrs_rev in file "src/clang/cTrans.ml" (inlined), line 5365, characters 28-54
Called from ClangFrontend__CTrans.CTrans_funct.exec_trans_instrs in file "src/clang/cTrans.ml" (inlined), line 5389, characters 6-70
Called from ClangFrontend__CTrans.CTrans_funct.instructions_trans in file "src/clang/cTrans.ml", line 5451, characte
- 1 others
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libcudacxx/include/cuda/__argument/argument_bounds.h (2)
222-233: ⚡ Quick winsuggestion: Local variables
__static_lowestand__static_highestare not modified after initialization and should be declaredconstper project guidelines.template <class _ElementType, class _StaticBounds> _CCCL_API constexpr _ElementType __effective_lowest(__runtime_bounds<_ElementType> __runtime_bounds) noexcept { - auto __static_lowest = ::cuda::__argument::__wrapper_static_lowest<_ElementType, _StaticBounds>(); + const auto __static_lowest = ::cuda::__argument::__wrapper_static_lowest<_ElementType, _StaticBounds>(); return __static_lowest > __runtime_bounds.lower() ? __static_lowest : __runtime_bounds.lower(); } template <class _ElementType, class _StaticBounds> _CCCL_API constexpr _ElementType __effective_highest(__runtime_bounds<_ElementType> __runtime_bounds) noexcept { - auto __static_highest = ::cuda::__argument::__wrapper_static_highest<_ElementType, _StaticBounds>(); + const auto __static_highest = ::cuda::__argument::__wrapper_static_highest<_ElementType, _StaticBounds>(); return __static_highest < __runtime_bounds.upper() ? __static_highest : __runtime_bounds.upper(); }As per coding guidelines: "All variables that are not modified must use
const."
195-240: 💤 Low valuesuggestion: Several newly added functions with non-void return types are missing
[[nodiscard]]. Existing functions in this file (lines 57, 61, 94, 99, 125, 136, 162) use[[nodiscard]]for similar getters and computations. Consider adding it to__wrapper_static_lowest,__wrapper_static_highest,__effective_lowest,__effective_highest, and__valid_argument_boundsfor consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 01ae3aed-d87f-40a3-9791-b7ef0544db90
📒 Files selected for processing (6)
libcudacxx/include/cuda/__argument/argument.hlibcudacxx/include/cuda/__argument/argument_bounds.hlibcudacxx/test/libcudacxx/cuda/argument/argument_bounds.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp
| static_assert(__is_sequence_v<value_type>, "The value type of __constant_sequence must be a range or an array"); | ||
| static_assert(!::cuda::std::ranges::__empty::__fn{}(_Value), | ||
| "The value type of __constant_sequence must be a non-empty sequence"); |
There was a problem hiding this comment.
important: __constant_sequence now rejects empty sequences, but an existing test still expects empty sequences to compile.
Line 117 introduces a hard static_assert for non-empty sequences, while libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp still instantiates cuda::__argument::__constant_sequence<cuda::std::array<int, 0>{}> as a positive case (Lines 82-87 in that file). Please convert that case to a negative-compile test (or remove it), otherwise class-NTTP-enabled configurations will fail.
😬 CI Workflow Results🟥 Finished in 1h 08m: Pass: 80%/115 | Total: 19h 40m | Max: 41m 45s | Hits: 98%/285814See results here. |
1 participant
We should not allow empty sequences for
__constant_sequenceWe also do not need all the wrappers