Skip to content

Conversation

@aprokop
Copy link
Contributor

@aprokop aprokop commented May 3, 2025

The patch introduces three concepts: AccessTraits, Primitives (same as AccessTraits) and Predicates (AccessTraits + tag). I put them into Concepts namespace.

In general, it works fine. One corner point is how it interacts with CTAD. clang evaluates the right hand side of CTAD before testing the concept, which leads to suboptimal errors instead of ignoring the CTAD (see godbolt). GCC, on the other hand, has a better treatment and properly rejects CTAD due to the unsatisfied requirement.

@aprokop aprokop added the API User visible interface modifications label May 3, 2025
@aprokop aprokop requested review from dalg24 and masterleinad May 3, 2025 16:53
template <typename Traits>
using AccessTraitsMemorySpaceArchetypeAlias = typename Traits::memory_space;
template <class T, class U>
concept not_same_as = !std::same_as<T, U>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this because there does not seem to be a non-void concept in std, and !std::same_as<void> did not work.

@aprokop
Copy link
Contributor Author

aprokop commented May 16, 2025

@dalg24 Could you please take a look at this PR?

@aprokop aprokop force-pushed the access_traits_concepts branch from f07823d to 31bcb16 Compare July 2, 2025 21:35
@aprokop aprokop force-pushed the access_traits_concepts branch from 31bcb16 to e8e40e2 Compare July 2, 2025 22:39
@aprokop
Copy link
Contributor Author

aprokop commented Jul 22, 2025

Moved 'Concepts' namespace inside Details to hide them from users.

@aprokop
Copy link
Contributor Author

aprokop commented Oct 7, 2025

Here are what the errors look like for the tstCompileOnlyAccessTraits.cpp:

In LLVM:

<snip>/test/tstCompileOnlyAccessTraits.cpp:137:7: error: static assertion failed
  137 |       ArborX::Details::Concepts::AccessTraits<NoAccessTraitsSpecialization>);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<snip>/test/tstCompileOnlyAccessTraits.cpp:137:7: note: because 'NoAccessTraitsSpecialization' does not satisfy 'AccessTraits'
<snip>/src/spatial/detail/ArborX_AccessTraits.hpp:82:37: note: because 'typename ArborX::AccessTraits<T>::memory_space' would be invalid: no type named 'memory_space' in 'ArborX::AccessTraits<NoAccessTraitsSpecialization>'
   82 |   typename ArborX::AccessTraits<T>::memory_space;

<snip>/test/tstCompileOnlyAccessTraits.cpp:138:17: error: static assertion failed
  138 |   static_assert(ArborX::Details::Concepts::AccessTraits<EmptySpecialization>);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<snip>/test/tstCompileOnlyAccessTraits.cpp:138:17: note: because 'EmptySpecialization' does not satisfy 'AccessTraits'
<snip>/src/spatial/detail/ArborX_AccessTraits.hpp:82:37: note: because 'typename ArborX::AccessTraits<T>::memory_space' would be invalid: no type named 'memory_space' in 'ArborX::AccessTraits<EmptySpecialization>'
   82 |   typename ArborX::AccessTraits<T>::memory_space;

<snip>/test/tstCompileOnlyAccessTraits.cpp:139:17: error: static assertion failed
  139 |   static_assert(ArborX::Details::Concepts::AccessTraits<InvalidMemorySpace>);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<snip>/test/tstCompileOnlyAccessTraits.cpp:139:17: note: because 'InvalidMemorySpace' does not satisfy 'AccessTraits'
<snip>/src/spatial/detail/ArborX_AccessTraits.hpp:83:12: note: because 'Kokkos::is_memory_space_v<typename ArborX::AccessTraits<InvalidMemorySpace>::memory_space>' evaluated to false
   83 |   requires Kokkos::is_memory_space_v<

<snip>/test/tstCompileOnlyAccessTraits.cpp:141:7: error: static assertion failed
  141 |       ArborX::Details::Concepts::AccessTraits<MissingSizeMemberFunction>);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<snip>/test/tstCompileOnlyAccessTraits.cpp:141:7: note: because 'MissingSizeMemberFunction' does not satisfy 'AccessTraits'
<snip>/src/spatial/detail/ArborX_AccessTraits.hpp:87:22: note: because 'AccessTraits<T>::size(v)' would be invalid: no member named 'size' in 'ArborX::AccessTraits<MissingSizeMemberFunction>'
   87 |     AccessTraits<T>::size(v)

<snip>/test/tstCompileOnlyAccessTraits.cpp:143:7: error: static assertion failed
  143 |       ArborX::Details::Concepts::AccessTraits<SizeMemberFunctionNotStatic>);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<snip>/test/tstCompileOnlyAccessTraits.cpp:143:7: note: because 'SizeMemberFunctionNotStatic' does not satisfy 'AccessTraits'
<snip>/src/spatial/detail/ArborX_AccessTraits.hpp:87:22: note: because 'AccessTraits<T>::size(v)' would be invalid: call to non-static member function without an object argument
   87 |     AccessTraits<T>::size(v)

<snip>/test/tstCompileOnlyAccessTraits.cpp:145:7: error: static assertion failed
  145 |       ArborX::Details::Concepts::AccessTraits<MissingGetMemberFunction>);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<snip>/test/tstCompileOnlyAccessTraits.cpp:145:7: note: because 'MissingGetMemberFunction' does not satisfy 'AccessTraits'
<snip>/src/spatial/detail/ArborX_AccessTraits.hpp:91:20: note: because 'AccessTraits<T>::get(v, 0)' would be invalid: no member named 'get' in 'ArborX::AccessTraits<MissingGetMemberFunction>'
   91 |   AccessTraits<T>::get(v, 0);

<snip>/test/tstCompileOnlyAccessTraits.cpp:147:7: error: static assertion failed
  147 |       ArborX::Details::Concepts::AccessTraits<GetMemberFunctionNotStatic>);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<snip>/test/tstCompileOnlyAccessTraits.cpp:147:7: note: because 'GetMemberFunctionNotStatic' does not satisfy 'AccessTraits'
<snip>/src/spatial/detail/ArborX_AccessTraits.hpp:91:20: note: because 'AccessTraits<T>::get(v, 0)' would be invalid: call to non-static member function without an object argument
   91 |   AccessTraits<T>::get(v, 0);

<snip>/test/tstCompileOnlyAccessTraits.cpp:148:17: error: static assertion failed
  148 |   static_assert(ArborX::Details::Concepts::AccessTraits<GetMemberFunctionVoid>);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<snip>/test/tstCompileOnlyAccessTraits.cpp:148:17: note: because 'GetMemberFunctionVoid' does not satisfy 'AccessTraits'
<snip>/src/spatial/detail/ArborX_AccessTraits.hpp:92:6: note: because '!requires (const GetMemberFunctionVoid &v) { { AccessTraits<GetMemberFunctionVoid>::get(v, 0) } -> std::same_as<void>; }' evaluated to false
   92 | } && !requires(T const &v) {

So I see two suboptimal messages:

  1. If specialization is undefined, we don't explicitly say that
  2. If specialization returns void, the error is a bit challenging to parse

@aprokop
Copy link
Contributor Author

aprokop commented Oct 7, 2025

It seems that the details of notes seem to depend on whether static_assert does any computations. For example,
static_assert(ArborX::Details::Concepts::AccessTraits<NoAccessTraitsSpecialization>); fails with

/tstCompileOnlyAccessTraits.cpp.o -MF test/CMakeFiles/ArborX_Test_CompileOnly.exe.dir/tstCompileOnlyAccessTraits.cpp.o.d -o test/CMakeFiles/ArborX_Test_CompileOnly.exe.dir/tstCompileOnlyAccessTraits.cpp.o -c <snip>/test/tstCompileOnlyAccessTraits.cpp
<snip>/test/tstCompileOnlyAccessTraits.cpp:137:7: error: static assertion failed
  137 |       ArborX::Details::Concepts::AccessTraits<NoAccessTraitsSpecialization>);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<snip>/test/tstCompileOnlyAccessTraits.cpp:137:7: note: because 'NoAccessTraitsSpecialization' does not satisfy 'AccessTraits'
<snip>/src/spatial/detail/ArborX_AccessTraits.hpp:82:37: note: because 'typename ArborX::AccessTraits<T>::memory_space' would be invalid: no type named 'memory_space' in 'ArborX::AccessTraits<NoAccessTraitsSpecialization>'
   82 |   typename ArborX::AccessTraits<T>::memory_space;
      |                                     ^                                   ^

but
static_assert(!(!ArborX::Details::Concepts::AccessTraits<NoAccessTraitsSpecialization>)); fails simply with

<snip>/test/tstCompileOnlyAccessTraits.cpp:136:17: error: static assertion failed due to requirement '!(!ArborX::Details::Concepts::AccessTraits<NoAccessTraitsSpecialization>)'
  136 |   static_assert(!(
      |                 ^~
  137 |       !ArborX::Details::Concepts::AccessTraits<NoAccessTraitsSpecialization>));
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Which is fine, but is still interesting.

@aprokop
Copy link
Contributor Author

aprokop commented Oct 8, 2025

@dalg24 I added couple commits:

  1. Added a more thorough testing for Predicates concept, which also made is_valid_predicate_tag a concept
  2. Added complete_type concept which checks whether specialization exists. I'm open to a better name.

I do agree with you that if the concepts allowed to specify our error messages, that would be ideal. The only step in that direction I've seen is the [EWG] P1267, but it went nowhere, it seems. The only alternative I see is to name each step of our concepts with some meaningful name, which is a pain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API User visible interface modifications

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants