kokkos / mdspan

Reference implementation of mdspan targeting C++23
Other
413 stars 69 forks source link

Compilation failure using clang++ 15 and 16 #361

Closed tpadioleau closed 1 week ago

tpadioleau commented 2 weeks ago

Hi, I came across a compilation issue with clang++ versions 15 and 16 in C++2b mode. The compilation fails when expanding a parameter pack inside the bracket operator, here is a reproducer on [godbolt](https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:13,fontUsePx:'0',j:1,lang:c%2B%2B,selection:(endColumn:1,endLineNumber:20,positionColumn:1,positionLineNumber:20,selectionStartColumn:1,selectionStartLineNumber:20,startColumn:1,startLineNumber:20),source:'%23include+%3Carray%3E%0A%0A%23if+defined(__cpp_multidimensional_subscript)%0A%0A%23define+VERSION+2%0A%0A%23if+VERSION+%3D%3D+1%0A%23define+CALL_BRACKET_OP(container,+...)+container%5B__VA_ARGS__%5D%0A%23elif+VERSION+%3D%3D+2%0Adecltype(auto)+CALL_BRACKET_OP(auto+const%26+container,+auto...+args)+%7B%0A++++return+container%5Bargs...%5D%3B%0A%7D%0A%23endif%0A%0Aint+main()+%7B%0A++++std::array+const+a%7B0%7D%3B%0A++++return+CALL_BRACKET_OP(a,+0)%3B%0A%7D%0A%23endif%0A'),l:'5',n:'1',o:'C%2B%2B+source+%231',t:'0')),k:54.385964912280706,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:clang1500,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'0',intel:'0',libraryCode:'0',trim:'1',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,libs:!(),options:'-std%3Dc%2B%2B2b',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+clang+15.0.0+(Editor+%231)',t:'0')),k:50,l:'4',m:58.48234754699679,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'clang+rocm-5.7.0',editorid:1,fontScale:14,fontUsePx:'0',j:1,wrap:'0'),l:'5',n:'0',o:'Output+of+x86-64+clang+15.0.0+(Compiler+%231)',t:'0')),header:(),l:'4',m:41.51765245300321,n:'0',o:'',s:0,t:'0')),k:45.614035087719294,l:'3',n:'0',o:'',t:'0')),l:'2',n:'0',o:'',t:'0')),version:4).

Should we add some extra logic to define MDSPAN_USE_BRACKET_OPERATOR=0 in this case ? For now we only rely on __cpp_multidimensional_subscript, see https://github.com/kokkos/mdspan/blob/260f525ed71669e5dcf2438622d2c433b3e5c281/include/experimental/__p0009_bits/config.hpp#L241-L247

mhoemmen commented 2 weeks ago

Thanks for reporting this! Please feel free to submit a pull request. Did you have something like the following in mind?

 #ifndef MDSPAN_USE_BRACKET_OPERATOR 
 #  if defined(__cpp_multidimensional_subscript)
 #    if defined(_MDSPAN_COMPILER_CLANG) && ((__clang_major__ == 15) || (__clang_major__ == 16))
 #      define MDSPAN_USE_BRACKET_OPERATOR 0
 #    else
 #      define MDSPAN_USE_BRACKET_OPERATOR 1
 #    endif
 #  else 
 #    define MDSPAN_USE_BRACKET_OPERATOR 0 
 #  endif 
 #endif 
tpadioleau commented 2 weeks ago

Yes exactly, I can open a PR. How do we keep track on why we introduce this new logic ? A comment in the source ?

mhoemmen commented 2 weeks ago

@tpadioleau Including a comment in the source to explain the new logic would be an excellent idea -- thank you!