-
Notifications
You must be signed in to change notification settings - Fork 216
Implement operator<<
for cuda::std::string_view
#4736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
||
_CCCL_EXEC_CHECK_DISABLE | ||
template <class _CharT, class _SizeT, class _Traits, _SizeT __npos> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding _CCCL_EXEC_CHECK_DISABLE
is necessary to allow using e. g. host only char_traits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit torn here, we should use host only char_traits
as they will crash.
I believe the solution with just casting to ::std::string_view
is the better solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the user can pass any type traits he wants, they might be host only, or device only..
template <class _CharT, class _Traits> | ||
_LIBCUDACXX_HIDE_FROM_ABI ::std::basic_ostream<_CharT, _Traits>& | ||
_CCCL_HIDE_FROM_ABI _CCCL_HOST ::std::basic_ostream<_CharT, _Traits>& | ||
operator<<(::std::basic_ostream<_CharT, _Traits>& __os, basic_string_view<_CharT, _Traits> __str) | ||
{ | ||
return __os.write(__str.data(), static_cast<::std::streamsize>(__str.size())); | ||
return __os << ::std::basic_string_view<_CharT, _Traits>{__str.data(), __str.size()}; | ||
} | ||
|
||
template <class _CharT> | ||
_CCCL_HIDE_FROM_ABI _CCCL_HOST ::std::basic_ostream<_CharT, ::std::char_traits<_CharT>>& operator<<( | ||
::std::basic_ostream<_CharT, ::std::char_traits<_CharT>>& __os, basic_string_view<_CharT, char_traits<_CharT>> __str) | ||
{ | ||
return __os << ::std::basic_string_view<_CharT, ::std::char_traits<_CharT>>{__str.data(), __str.size()}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had to split the implementation to map the cuda::std::char_traits
to std::char_traits
|
||
_CCCL_EXEC_CHECK_DISABLE | ||
template <class _CharT, class _SizeT, class _Traits, _SizeT __npos> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit torn here, we should use host only char_traits
as they will crash.
I believe the solution with just casting to ::std::string_view
is the better solution
@@ -147,6 +147,21 @@ public: | |||
, __size_{_CUDA_VRANGES::size(__r)} | |||
{} | |||
|
|||
#if !_CCCL_COMPILER(NVRTC) | |||
_CCCL_HIDE_FROM_ABI constexpr basic_string_view(::std::basic_string_view<_CharT, _Traits> __sv) noexcept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This forces use of the same char_traits than this string_view is using. We should templatize this so that we can construct from a ::std::string_view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional, the idea is that both char traits types must match with an exception of std::char_traits
that are mapped to cuda::std::char_traits
. That means that:
cuda::std::basic_string_view<CharT, cuda::std::char_traits<CharT>>
can be constructed (converted) from (to) bothstd::basic_string_view<CharT, std::char_traits<CharT>>
andstd::basic_string_view<CharT, cuda::std::char_traits<CharT>>
cuda::std::basic_string_view<CharT, Traits>
can be constructed (converted) from (to)std::basic_string_view<CharT, Traits>
Do you think this is wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe then should have 2 constructors,
- basic_string_view(::std::basic_string_view<_CharT, _Traits> __sv)
- basic_string_view(::std::basic_string_view<_CharT, ::std::char_traits<_CharT>> __sv)
We can then omit the constraint on the second constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what we have now. But I don't think we can omit the constraint, because we must ensure that the current type traits are cuda::std::char_traits
, otherwise we would allow something like:
cuda::std::basic_string_view<my_char, my_char_traits>{std::basic_string_view<my_char, std::char_traits<my_char>>{...}};
which is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think so, the incoming type traits are correct and its the users responsibility to ensure that their char_traits are conforming.
Also no one writes their own char_traits, so I believe its more of a theoretical question
031d5fa
to
576a28c
Compare
/ok to test 576a28c |
/ok to test 12ebede |
/ok to test d8346ae |
/ok to test 2974a4f |
🟨 CI finished in 4h 38m: Pass: 94%/183 | Total: 1d 09h | Avg: 10m 52s | Max: 41m 07s | Hits: 95%/270797
|
Project | |
---|---|
CCCL Infrastructure | |
+/- | libcu++ |
CUB | |
Thrust | |
CUDA Experimental | |
stdpar | |
python | |
CCCL C Parallel Library | |
Catch2Helper |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | stdpar |
+/- | python |
+/- | CCCL C Parallel Library |
+/- | Catch2Helper |
🏃 Runner counts (total jobs: 183)
# | Runner |
---|---|
129 | linux-amd64-cpu16 |
15 | windows-amd64-cpu16 |
12 | linux-arm64-cpu16 |
12 | linux-amd64-gpu-rtxa6000-latest-1 |
7 | linux-amd64-gpu-rtx2080-latest-1 |
5 | linux-amd64-gpu-h100-latest-1 |
3 | linux-amd64-gpu-rtx4090-latest-1 |
This PR implements
operator<<
forcuda::std::string_view
. This is the final piece of<cuda/std/string_view>
implementation, so I've also updated the version macro.