[libcu++] Fix __is_sequence definition for arguments framework#9271
[libcu++] Fix __is_sequence definition for arguments framework#9271miscco wants to merge 3 commits into
__is_sequence definition for arguments framework#9271Conversation
OverviewThis PR replaces 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 some wrappers as sequences, and preventing iterators/pointers from being passed as single values). The new approach narrows what is considered a sequence to arrays, types meeting ranges::range, or types that provide random-access traversal. Tests were added/adjusted to lock in the intended behavior and avoid regressions. ChangesImplementation (libcudacxx/include/cuda/__argument/argument.h)
Tests
Rationale & Impact
Notes for reviewers
Walkthroughsuggestion: Reclassifies 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/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/usage_example.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/usage_example.pass.cpp:16:10: fatal error: 'cuda/_argument' file not found ... [truncated 2200 characters] ... 3 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 |
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
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
😬 CI Workflow Results🟥 Finished in 1h 04m: Pass: 99%/115 | Total: 21h 05m | Max: 33m 37s | Hits: 98%/343670See 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