search

kokkos / mdspan

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

Build failing on GCC arm due to _N defined as a macro #187

Closed LnnrtS closed 1 year ago

LnnrtS commented 2 years ago

ctype.h on that platform defines

#define _U  01
#define _L  02
#define _N  04
#define _S  010
#define _P  020
#define _C  040
#define _X  0100
#define _B  0200

which breaks mdspan in many places since some of those names are used as template parameters.

Names starting with an underscore and upper case letter are reserved by the standard to be used by an implementation so I think the proper way to handle this is rename the template parameters. I didn't find a quick way to automate the renaming otherwise I would have made a pull request.

crtrott commented 2 years ago

Yeah this implementation is meant to go into the standard implementation hence we also use underscores, obviously we need to make it compatible with the ones we want to intregrate with. But seriously who injects single letter macros into the global namespace lol, almost as bad as MSVC definine MIN and MAX as macros ....

crtrott commented 2 years ago

On the other side I don't think there is a reason we should put _ in front of tempalte parameters in the first place.

mhoemmen commented 2 years ago

On the other side I don't think there is a reason we should put _ in front of tempalte parameters in the first place.

Right, I don't think it's our job necessarily to pre-mangle symbols. If a standard library implementation wants to adopt our reference mdspan implementation, they will do that mangling on their own.

This is a bit relevant to #185, though. We've been wanting to use a prefix _ as a way to mark macros as implementation details. On the other hand, "namespaced" macros like _MDSPAN_INLINE_FUNCTION probably won't collide with an existing standard library implementation's symbols.

LnnrtS commented 2 years ago

Correct me if I'm wrong but I don't think the name of template parameters have to be mangled at all because they are no symbols and they don't leave the scope of the template definition. But - as we can see in this particular issue - they are affected by global macros. So consequently I would say they actually may not be mangled at all.

mhoemmen commented 2 years ago

@LnnrtS wrote:

Correct me if I'm wrong but I don't think the name of template parameters have to be mangled at all because they are no symbols and they don't leave the scope of the template definition.

Just to clarify: The library in this repository does not need to mangle template parameters. Standard Library implementations of mdspan (which are coming) generally mangle symbols to avoid collisions with users' macros.

LnnrtS commented 2 years ago

Ok I see. I wasn't taking user defined macros into account (I mean, why would anybody.. ;-))

crtrott commented 2 years ago

We may wanna mangle but just use some larger names for the template parameters. That said I still think its insane to have in any library single letter macros, mangled or not.

burnpanck commented 1 year ago

Did just run into this myself (also on arm bare-metal). I too, am aghast to hear of those single letter macros (which I wasn't aware of). Unfortunately, bare-metal development still is very much a C world, and you'd be surprised how much ugliness can be found in manufacturer's toolchains and SDKs. So here too, I'd would be grateful for any other choice of template parameter names, despite the fact that these macros being a crime against any sane programmer.

Unfortunately, these macros are actually used somewhere deep inside internal headers of the stdlib, so the best way I found to avoid them leaking into the rest of the code is to include some significant part of the stdlib (<ios> - something that I really don't need on embedded) early on in every file and then undefining the offending macros.

crtrott commented 1 year ago

This PR I think will actually fix this incidentally: https://github.com/kokkos/mdspan/pull/212

crtrott commented 1 year ago

I think this is now fixed can you guys try? @burnpanck @LnnrtS

LnnrtS commented 1 year ago

Fixed, apart from _U in __compressed_pair

ehntoo commented 1 year ago

A quick godbolt reproduction demonstrating the remaining breakage in __compressed_pair: https://godbolt.org/z/s6qzafWP4

I can put together a PR to fix the remaining issue, but I've got two questions:

mhoemmen commented 1 year ago

Would an #ifdef _T1 ... #error "private macro already defined" -type check be welcomed?

This check would require 3 times (number-of-template-parameters) lines of preprocessor logic for every class template and function template. There are so many class templates and function templates in this library. An important goal of the reference implementation of mdspan is to be legible.

If we find ourselves wanting to work around this issue, I would much prefer just picking different template parameter names.

Would renaming __compressed_pair's _T and _U template parameters to _T1 and _T2 be an acceptable fix?

I would find that acceptable.