[libcu++] Fix __is_sequence definition for arguments framework#9271
[libcu++] Fix __is_sequence definition for arguments framework#9271miscco wants to merge 1 commit into
__is_sequence definition for arguments framework#9271Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
OverviewThis PR fixes the ChangesImplementation (
|
| Layer / File(s) | Summary |
|---|---|
Core implementation: sequence and traits reclassification libcudacxx/include/cuda/__argument/argument.h |
Adds range/type-trait includes; redefines __is_sequence_v<_Tp> as `::cuda::std::is_array_v<::cuda::std::remove_cvref_t<_Tp>> |
Test validation of classification changes libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp, libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp, libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp |
Updates test includes and static_assert expectations: int*, counting_iterator, and wrapper *_like types are now non-sequences; arrays and span/array remain sequences; inverts several is_single_value checks to use !__is_sequence_v instead of the removed predicate. |
-
Possibly related PRs:
- NVIDIA/cccl#8875: Updates
argument.hsequence/single-value trait logic similar to this PR. - NVIDIA/cccl#9074: Refactors consumers to rely on
::cuda::__argument::__traits<>affected by these trait changes.
- NVIDIA/cccl#8875: Updates
-
Suggested reviewers:
- ericniebler
- Jacobfaib
- gevtushenko
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_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/f13ff250a89da6bae7e336f74c8f3f19aecde1fb-07842b3345a172fc/tmp/clang_command_.tmp.a0d1ef.txt
++Contents of '/tmp/coderabbit-infer/f13ff250a89da6bae7e336f74c8f3f19aecde1fb-07842b3345a172fc/tmp/clang_command.tmp.a0d1ef.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
libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp
libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found
11 | #include <cuda/_argument>
| ^~~~~~~~~~~~~~~~~~
1 error generated.
libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp:124:3-128:3: ERROR translating statement 'CompoundStmt'
Aborting translation of method 'test' in file 'libcudacxx/test/libcudacxx/cuda/argument/static_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_ma
... [truncated 2200 characters] ...
Frontend__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, characters 25-68
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.add_method.f in file "src/clang/cFrontend_decl.ml", line 48, characters 12-91
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libcudacxx/include/cuda/__argument/argument.h (1)
114-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winimportant: The static_assert error messages "must have a distinct element type" are now incorrect. The new
__is_sequence_vchecks whether a type is an array or satisfiesranges::range, not whether it has a distinct element type. Types likeelement_type_like<int>have a distinct element type but are not arrays or ranges, so they would fail this check for a different reason than the message suggests.Update the messages to reflect the actual requirement, e.g., "must be an array or range type".
Proposed fix
- static_assert(__is_sequence_v<value_type>, "constant sequence arguments must have a distinct element type"); + static_assert(__is_sequence_v<value_type>, "constant sequence arguments must be an array or range type");Apply similar changes to lines 304, 701, and 714.
Also applies to: 304-304, 701-701, 714-714
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6b49057a-ad0c-4843-8771-66b0fd1e7ae9
📒 Files selected for processing (4)
libcudacxx/include/cuda/__argument/argument.hlibcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp
miscco
force-pushed
the
fix_argument_sequence_definition
branch
from
171cb93 to
90a8c02
Compare
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
miscco
force-pushed
the
fix_argument_sequence_definition
branch
from
90a8c02 to
f13ff25
Compare
|
I have added some more tests to make sure this does not regresses again. I believe we can agree that |
😬 CI Workflow Results🟥 Finished in 2h 01m: Pass: 91%/115 | Total: 19h 55m | Max: 1h 53m | Hits: 98%/338419See results here. |
1 participant
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