[libcu++] Fix __is_sequence definition for arguments framework#9271
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:
OverviewThis PR fixes a broken sequence-detection trait used by the arguments framework. The previous element-type-based logic misclassified many types (e.g., treating tuple/complex-like types and certain wrapper-like types as sequences and preventing iterators/pointers from being passed as single values). The new definition narrows what is considered a sequence to arrays, types satisfying ranges::range, or types that provide random-access traversal. Tests were added/updated to lock in this behavior and avoid regressions. ChangesImplementation (libcudacxx/include/cuda/__argument/argument.h)
Tests
Rationale & Impact
Notes for reviewers
suggestion: WalkthroughReclassifies sequence detection: a sequence is now an array (after cv/ref removal), a ChangesArgument type classification by ranges::range
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/dynamic_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found ... [truncated 2200 characters] ... from ClangFrontend__CTrans.CTrans_funct.exec_with_node_creation in file "src/clang/cTrans.ml" (inlined), line 104, characters 20-38 libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found ... [truncated 1169 characters] ... al-isystem" "/usr/local/include" "-internal-isystem" libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found ... [truncated 2200 characters] ... Frontend__CTrans.CTrans_funct.get_clang_stmt_trans in file "src/clang/cTrans.ml" (inlined), line 5395, characters 4-69
Comment |
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
90a8c02 to
f13ff25
Compare
|
I have added some more tests to make sure this does not regresses again. I believe we can agree that |
This comment has been minimized.
This comment has been minimized.
| inline constexpr bool __is_iterable_v<_Tp, | ||
| ::cuda::std::void_t<decltype(::cuda::std::declval<const _Tp&>().begin()), | ||
| decltype(::cuda::std::declval<const _Tp&>().end())>> = true; | ||
| ::cuda::std::is_array_v<::cuda::std::remove_cvref_t<_Tp>> || ::cuda::std::ranges::range<_Tp>; |
There was a problem hiding this comment.
An array is a range though, no? Why do you need both conditionals?
There was a problem hiding this comment.
Unfortunately not: https://godbolt.org/z/Eadh18x74
|
I am not tied to the name of this trait, but at the end of the day when user passes something that has bounds we need to understand if its one value or multiple values. We have the I highly doubt we can convince others CUB should stop accepting iterators and pointers, so I believe we need a broader definition than just range, but maybe we can also remove the extra validation, which would remove the need for this trait. But it is likely in that case a similar trait would need to live in CUB anyway, so I am not sure if it isn't just pushing the problem to a different place |
We should be able to still allow pointers and iterators if that is intended, but then the check should probably be Also we need to then differentiate iteration over a range and iteration over an iterator. Currently the check in The issue I have here is that a pointer / iterator does not communicate the size of the range. In a perfect world we would just pass Not a hill to die on definitely but I at least want to make sure that we do not consider |
f13ff25 to
c7f5b4b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libcudacxx/test/libcudacxx/cuda/argument/usage_example.pass.cpp (1)
85-94: ⚡ Quick winimportant: Line 85 disables the plain-span scenario with
#if 0, which drops coverage instead of asserting the intended rejection behavior. Replace this block with an explicit negative test (compile-fail/static assertion in the argument test suite) so the contract is enforced by CI rather than commented out.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a8f64be5-de33-471d-b9c3-6f0c1e6388d6
📒 Files selected for processing (3)
libcudacxx/include/cuda/__argument/argument.hlibcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/usage_example.pass.cpp
This comment has been minimized.
This comment has been minimized.
| if constexpr (__is_iterable_v<_Arg> && ::cuda::std::is_arithmetic_v<__element_type>) | ||
| if constexpr (::cuda::std::__has_random_access_traversal<_Arg>) | ||
| { // FIXME: (miscco) This is broken. we do not know the size of the sequence | ||
| __validate_element(*__arg_); |
There was a problem hiding this comment.
For this I think we should just make validate a separate call that takes a size, but it can happen in a separate PR
There was a problem hiding this comment.
I would like to merge as is, because this improves the status quo and we can handle further improvements in followup PRs
This comment has been minimized.
This comment has been minimized.
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
ce73d17 to
d0863c9
Compare
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/test/libcudacxx/cuda/argument/usage_example.pass.cpp (1)
85-94:⚠️ Potential issue | 🔴 Criticalsuggestion: The
#if 0-disabled “plain span” case leaves ambiguousFIXMEintent; the argument framework enforcesstatic_assert(__is_sequence_v<...>)only for the wrapper types (__constant_sequence/__immediate_sequence), and there’s no evidence here that an unwrappedcuda::std::spanis explicitly rejected at compile time. Either remove the dead block, or turn it into a negative/positive test that matches the framework’s actual contract (unwrapped spans should be either supported and wrapped correctly, or deliberately diagnosed via an expect-error test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a5817848-9eb4-4794-899d-6fb322be9bb1
📒 Files selected for processing (5)
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.cpplibcudacxx/test/libcudacxx/cuda/argument/usage_example.pass.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp
- libcudacxx/include/cuda/__argument/argument.h
- libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp
🥳 CI Workflow Results🟩 Finished in 1h 38m: Pass: 100%/118 | Total: 23h 21m | Max: 55m 15s | Hits: 98%/342444See results here. |
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