[libcu++] Improve is_nothrow_meow traits - #10991
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request relocates nothrow builtin detection into individual trait headers. It rewrites fallback assignability, constructibility, and destructibility evaluation with variable templates. Derived nothrow traits now use ChangesNothrow type traits
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change only relocates and simplifies nothrow trait builtins; the remaining concern is limited to preprocessor comment consistency, with no actionable merge-blocking risk beyond normal review. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libcudacxx/include/cuda/std/__type_traits/is_nothrow_assignable.h (1)
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Repeat the complete originating
#ifcondition in each annotated#elseand#endifcomment.
libcudacxx/include/cuda/std/__type_traits/is_nothrow_assignable.h#L30-L32: Include the MSVC and NVRTC clauses in the#endifcomment.libcudacxx/include/cuda/std/__type_traits/is_nothrow_assignable.h#L36-L46: Repeat the completedefined(...) && !defined(...)condition in the#elsecomment.libcudacxx/include/cuda/std/__type_traits/is_nothrow_constructible.h#L30-L32: Include the MSVC and NVRTC clauses in the#endifcomment.libcudacxx/include/cuda/std/__type_traits/is_nothrow_destructible.h#L33-L35: Include the MSVC and NVRTC clauses in the#endifcomment.libcudacxx/include/cuda/std/__type_traits/is_nothrow_destructible.h#L41-L50: Repeat the completedefined(...) && !defined(...)condition in the#elsecomment.Based on learnings, annotated preprocessor comments must repeat the exact corresponding condition text.
Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f455d543-2a82-46f2-ac06-9f8a6c1cd5a5
📒 Files selected for processing (9)
libcudacxx/include/cuda/std/__cccl/builtin.hlibcudacxx/include/cuda/std/__type_traits/is_nothrow_assignable.hlibcudacxx/include/cuda/std/__type_traits/is_nothrow_constructible.hlibcudacxx/include/cuda/std/__type_traits/is_nothrow_copy_assignable.hlibcudacxx/include/cuda/std/__type_traits/is_nothrow_copy_constructible.hlibcudacxx/include/cuda/std/__type_traits/is_nothrow_default_constructible.hlibcudacxx/include/cuda/std/__type_traits/is_nothrow_destructible.hlibcudacxx/include/cuda/std/__type_traits/is_nothrow_move_assignable.hlibcudacxx/include/cuda/std/__type_traits/is_nothrow_move_constructible.h
💤 Files with no reviewable changes (1)
- libcudacxx/include/cuda/std/__cccl/builtin.h
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.
| struct __cccl_is_nothrow_assignable<false, _Tp, _Arg> : public false_type | ||
| {}; | ||
| template <class _Tp, class _Arg, bool _IsAssignable = is_assignable_v<_Tp, _Arg>> | ||
| inline constexpr bool __cccl_is_nothrow_assignable_v = false; |
There was a problem hiding this comment.
Why the extra bool? Can't we just define this as is_nothrow_assignable_v?
| template <bool, class _Tp, class... _Args> | ||
| inline constexpr bool __cccl_is_nothrow_constructible = false; | ||
| template <bool _IsConstructible, class _Tp, class... _Args> | ||
| inline constexpr bool __cccl_is_nothrow_constructible_v = false; |
There was a problem hiding this comment.
Same here, let's just define this as is_nothrow_constructible_v directly
| inline constexpr bool is_nothrow_copy_assignable_v = _CCCL_BUILTIN_IS_NOTHROW_ASSIGNABLE( | ||
| add_lvalue_reference_t<_Tp>, add_lvalue_reference_t<typename add_const<_Tp>::type>); | ||
| inline constexpr bool is_nothrow_copy_assignable_v = | ||
| _CCCL_BUILTIN_IS_NOTHROW_ASSIGNABLE(add_lvalue_reference_t<_Tp>, add_lvalue_reference_t<add_const_t<_Tp>>); | ||
|
|
||
| #else | ||
|
|
||
| template <class _Tp> | ||
| struct _CCCL_TYPE_VISIBILITY_DEFAULT is_nothrow_copy_assignable |
There was a problem hiding this comment.
Question: can we just define this as a using decl a la
using is_nothrow_copy_assignable = is_nothrow_assignable<add_lvalue_reference_t<_Tp>, add_lvalue_reference_t<add_const_t<_Tp>>>;? Is it required to be a distinct type? Maybe @miscco would know this as well
There was a problem hiding this comment.
No, The standard requires this to be a separate type
There was a problem hiding this comment.
I dont think it needs to be a separate type
That said this would definitely prevent people from specializing it. Given that its almost never used at all internally I would keep it as is
There was a problem hiding this comment.
That said this would definitely prevent people from specializing it.
Specializing it is expressly disallowed by the standard though.
😬 CI Workflow Results🟥 Finished in 4h 26m: Pass: 88%/99 | Total: 5d 02h | Max: 4h 26m | Hits: 8%/257247See results here. |
3 participants
This PR moves
is_nothrow_meowbuiltins directly to the files and reduces the number of type instantiations.