Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[libc++] __bit_reference template parameter conflicting with netcdf #80560

Closed
DimitryAndric opened this issue Feb 3, 2024 · 4 comments
Closed
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. quality-of-implementation

Comments

@DimitryAndric
Copy link
Collaborator

As of 4d20cfc, __bit_reference contains a template __fill_n with a bool _FillValue parameter.

Unfortunately there is a relatively widely used piece of scientific software called NetCDF, which exposes a (C) macro _FillValue in its public headers: https://github.com/Unidata/netcdf-c/blob/main/include/netcdf.h#L113 .

When building the NetCDF C++ bindings this quickly leads to compilation errors when the macro interferes with the template in __bit_reference.

I have mentioned this to them in Unidata/netcdf-c#2858, but it looks like it will be pretty difficult to change, not in the least because it is likely that external consumers of this library also depend on the macro.

Of course identifiers starting with underscores are reserved, but in this case it might not be that impactful for libc++ to rename the template parameter to something slightly different, and non-conflicting? I would count this as a minor QoL fix.

CC @philnik777 who is the author of 4d20cfc.

@DimitryAndric DimitryAndric added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. quality-of-implementation labels Feb 3, 2024
@philnik777
Copy link
Contributor

I'm fine with it if you want to provide a small patch. This should still be fixed by the library to avoid future problems. @AaronBallman is there a reason not even -Wreserved-macro-identifier is enabled by default, given that macros are by far the most egregious violations of the reserved namespace? This seems like the kind of thing that could be easily avoided if people knew they are using identifiers they aren't allowed to use.

@DimitryAndric
Copy link
Collaborator Author

Pull request: #80661

@AaronBallman
Copy link
Collaborator

I'm fine with it if you want to provide a small patch. This should still be fixed by the library to avoid future problems. @AaronBallman is there a reason not even -Wreserved-macro-identifier is enabled by default, given that macros are by far the most egregious violations of the reserved namespace? This seems like the kind of thing that could be easily avoided if people knew they are using identifiers they aren't allowed to use.

We disable -Wreserved-macro-identifier by default because a popular IDE which shall remain nameless used to auto-insert header guards using reserved identifiers, so the diagnostic was going to be extremely chatty. However, it might make sense to split the diagnostic up into another level of granularity we'd have:

-Wreserved-macro-identifier-strict (off by default)
  -Wreserved-macro-identifier-header-guard (off by default)
  -Wreserved-macro-identifier (on by default)

(I'd have to give it more thought.) Because you're right, this problem comes up quite often and I think we need to start being more aggressive about warning on reserved identifiers in general. (A reservation only works in practice if users actually know about it!)

@DimitryAndric
Copy link
Collaborator Author

Fixed with 1ec2522.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. quality-of-implementation
Projects
None yet
Development

No branches or pull requests

3 participants