ArborX icon indicating copy to clipboard operation
ArborX copied to clipboard

Introduce concepts for AccessTraits

Open aprokop opened this issue 8 months ago • 5 comments

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 avatar May 03 '25 16:05 aprokop

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

aprokop avatar May 16 '25 20:05 aprokop

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

aprokop avatar Jul 22 '25 15:07 aprokop

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 avatar Oct 07 '25 19:10 aprokop

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 avatar Oct 07 '25 20:10 aprokop

@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.

aprokop avatar Oct 08 '25 12:10 aprokop