Compilation with -DVL=... crashes in type_list.hpp

[kbobrovs]

This code in CL/sycl/detail/type_list.hpp template <access::address_space AS, typename VL> causes compilation error if user specifies -DVL=<some number> option on the command line. VL is a popular acronym for "vector length", people may use it as a macro in their programs.

Names which can clash with user space should start with underscore(s). E.g. template <access::address_space __AS, typename __VL>.

[al42and]

Realted: DIM and NDIMS (from sycl/include/CL/sycl/detail/common.hpp) are also moderately common as macro names.

[AaronBallman]

All of this library code is part of the implementation, so I think all of the identifiers (aside from parts of the public API other than parameter names) should be in the reserved namespace, correct?

[kbobrovs]

All of this library code is part of the implementation, so I think all of the identifiers (aside from parts of the public API other than parameter names) should be in the reserved namespace, correct?

I think this is already the case (sycl::detail namespace is used), bit we can't protect template formal argument names with namespace from being redefined with #define or -D compiler switch.

[AaronBallman]

All of this library code is part of the implementation, so I think all of the identifiers (aside from parts of the public API other than parameter names) should be in the reserved namespace, correct?

I think this is already the case (sycl::detail namespace is used), bit we can't protect template formal argument names with namespace from being redefined with #define or -D compiler switch.

Namespaces don't solve the issue with macros though. The user can still #define or -D other identifiers that cause problems. e.g., (based on https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/detail/type_list.hpp#L42) they could -Dremainder=const and this would still break the header.

[kbobrovs]

All of this library code is part of the implementation, so I think all of the identifiers (aside from parts of the public API other than parameter names) should be in the reserved namespace, correct?

I think this is already the case (sycl::detail namespace is used), bit we can't protect template formal argument names with namespace from being redefined with #define or -D compiler switch.

Namespaces don't solve the issue with macros though. The user can still #define or -D other identifiers that cause problems. e.g., (based on https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/detail/type_list.hpp#L42) they could -Dremainder=const and this would still break the header.

Yes, that's exactly what I meant. So the suggested solution is to use __ prefix for template argument names. I think that's what other compiler vendors do (e.g. Microsoft). We can't protect all the identifiers that way obviously, as this would be impractical, but all-capital ones which are acronyms with high probability of redefinition can be protected.

[AaronBallman]

My suggestion is to use __ prefix for all names that are not exposed to the user as part of the public API. e.g., go with regular identifiers for classes, public class members, enumeration constants, etc but go with ugly names for template parameters, function parameters, private members of a class, implementation details, etc. This is a common strategy for other library code: https://github.com/llvm/llvm-project/blob/main/libcxx/include/string#L722

[kbobrovs]

Adding @bader.

[bader]

I might be wrong, but heard that libcxx is required to use __ prefix by C++ specification, which applies to rule to standard library implementations. User libraries are not required to follow this rule, but obviously library developers should think about how their libraries might be used. You can notice that other LLVM libraries do not use __ prefix - https://github.com/intel/llvm/blob/sycl/llvm/include/llvm/ADT/APFloat.h. A C++ compiler won't be able to compile this header with -DT=....
Although DPC++ library headers are distributed with the compiler, I'm not sure if we want to apply this requirement to our code base. It really hurts code readability.

Alternative solution might be to use CamelCase avoid conflicts with macro names which are usually use caps.
For the example from the issue description:

template <access::address_space AddrSpace, typename VecLen>
template <access::address_space AddressSpace, typename VectorLength>
// or any other variant with lower case characters

[AaronBallman]

STL libraries are not required to do this as a matter of specification, but they do it as a matter of Quality of Implementation because of just how often user-defined macros conflict with library code in practice.

User library code like APFloat.h doesn't go to these lengths because they basically can't. Identifiers that start with double underscores (or single underscore followed by a capital letter) are reserved to the implementation, so using such an identifier in APFloat.h (or any other user header) would be undefined behavior.

However, in this case, the SYCL headers are a part of the compiler implementation itself. These reservations are specifically for folks in our situation so that we don't have to risk conflicting with user code. Personally, I don't think a few extra underscores impacts readability to the point where we should be concerned about it. Going to PascalCase for the identifiers doesn't really solve the problem -- the only way to safely do it is to make use of the implementation's reserved identifier space, because any user who has a macro that starts with double underscores (or single underscore followed by a capital letter) has undefined behavior, so they get whatever mess they get from it.

[HabKaffee]

FYI, I created a PR which change rules of identifier naming #5843 .

[steffenlarsen]

For the sake of reviving this discussion, I agree with @AaronBallman that the only way to be fully out of the woods is to use reserved identifiers in our headers, otherwise we are just making assumptions about user code which could easily land us back in this very same discussion again when someone has -DVecLen=... in their compilation. I don't think we should rule out code-bases using different coding standards, but if they start using reserved names we can at least point them to the C++ Standard.

As for how we would enforce it, clang-tidy seems to have a bugprone-reserved-identifier we may be able to use in its inverted state to root out current and future identifiers not following these restrictions. One problem with it is that our public interface would need to be in the exclusion list (as well as identifiers used in symbols of our ABI until ABI break is possible) which is likely to make the exclusion list quite large and slightly annoying to maintain. I wonder if we could somehow extend it to ignore public identifiers within a certain namespace or something along those lines. If this is what we decide, I am open to ideas on this.

@kbobrovs @bader @AaronBallman - Thoughts?