Macro named '_FillValue' conflicts with libc++ headers

[DimitryAndric]

This is a report of a problem encountered when building the science/netcdf-cxx port on FreeBSD, against libc++ 18 headers. The reason I am filing an issue in this project instead, is that the actual error is caused by a problem in the netcdf.h header, which is included from netcdf-cxx.

When you attempt to build netcdf-cxx against installed netcdf-c headers, several compile errors are produced, similar to:

libtool: compile:  c++ -DHAVE_CONFIG_H -I. -I.. -fPIC -DPIC -isystem /usr/local/include -O2 -pipe -fstack-protector-strong -isystem /usr/local/include -fno-strict-aliasing -isystem /usr/local/include -MT ncAtt.lo -MD -MP -MF .deps/ncAtt.Tpo -c ncAtt.cpp  -fPIC -DPIC -o .libs/ncAtt.o
In file included from ncAtt.cpp:1:
In file included from ./ncAtt.h:2:
In file included from ./ncException.h:3:
In file included from /usr/include/c++/v1/iostream:44:
In file included from /usr/include/c++/v1/istream:170:
In file included from /usr/include/c++/v1/ostream:187:
In file included from /usr/include/c++/v1/bitset:133:
/usr/include/c++/v1/__bit_reference:176:16: error: expected ',' or '>' in template-parameter-list
  176 | template <bool _FillValue, class _Cp>
      |                ^
/usr/local/include/netcdf.h:113:25: note: expanded from macro '_FillValue'
  113 | #define _FillValue      "_FillValue"
      |                         ^
In file included from ncAtt.cpp:1:
In file included from ./ncAtt.h:2:
In file included from ./ncException.h:3:
In file included from /usr/include/c++/v1/iostream:44:
In file included from /usr/include/c++/v1/istream:170:
In file included from /usr/include/c++/v1/ostream:187:
In file included from /usr/include/c++/v1/bitset:133:
/usr/include/c++/v1/__bit_reference:176:26: error: expected unqualified-id
  176 | template <bool _FillValue, class _Cp>
      |                          ^
/usr/include/c++/v1/__bit_reference:1010:18: error: expected ',' or '>' in template-parameter-list
 1010 |   template <bool _FillValue, class _Dp>
      |                  ^
/usr/local/include/netcdf.h:113:25: note: expanded from macro '_FillValue'
  113 | #define _FillValue      "_FillValue"
      |                         ^
In file included from ncAtt.cpp:1:
In file included from ./ncAtt.h:2:
In file included from ./ncException.h:3:
In file included from /usr/include/c++/v1/iostream:44:
In file included from /usr/include/c++/v1/istream:170:
In file included from /usr/include/c++/v1/ostream:187:
In file included from /usr/include/c++/v1/bitset:133:
/usr/include/c++/v1/__bit_reference:1010:28: error: expected member name or ';' after declaration specifiers
 1010 |   template <bool _FillValue, class _Dp>
      |                            ^
4 errors generated.

The problem is that _FillValue is a macro defined in netcdf.h, but it conflicts with a template parameter name in libc++'s __bit_reference header. In general, identifiers starting with _ are reserved for the system headers, so in my opinion netcdf.h should be adjusted.

However, it looks like the macro name itself is used in a lot of places in netcdf itself, and possibly also in derived projects such as netcdf-cxx. Therefore, I would like to ask you how much breakage would be expected, if the macro name was changed to, for example, NC_FillValue ?

For reference, the FreeBSD science/netcdf port is at version 4.9.2, while the science/netcdf-cxx port is at version 4.3.1, but the problem is also present in the current main branch of this project.

[DennisHeimbigner]

The original _FillValue macro was, in hindsight, a mistake and should have had some tag such as NC_FillValue.
Unfortunately, it is difficult to change it at this point because of back compatibility.
The only thing I can suggest is that after you build and install netcdf, you remove/rename the macro
in the installed netcdf.h

[DimitryAndric]

Do you expect any external netcdf.h consumers to use the _FillValue macro?

(In FreeBSD's port system there are about 300 ports that depend on netcdf, so I have asked for a test-run with the macro renamed. We'll see how many of these other programs fail to build.)

[DennisHeimbigner]

The problem is that netcdf.h is the embodiment of the API for the netcdf library.
As such, we have to take great care in removing declarations from netcdf.h.

[dopplershift]

I think an argument can be made that we should just change it so that we're actually compatible with C99. The spec specifically says:

All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.

[seanm]

All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.

And you can find all violations of this rule with clang's -Wreserved-identifier warning flag.

[edwardhartnett]

I agree with @dopplershift that it should just be changed.

Sure, some user might be using the macro, but if so, they will discover the problem at their next compile, and it's trivial for them to fix it.

I suggest just change it, put it in the release notes, and move on. It can just be commented out in the file, and then if the user is using it, and wants to uncomment it as a temp fix, that would be easy.

[ldionne]

@DennisHeimbigner While I agree that changing API surface is always tricky, in this case NetCDF is using a reserved identifier and that's causing way more harm than the migration of your own users to the new API. For example, it could have easily been the case that your library simply doesn't build anymore when using a recent version of Clang, which effectively means that it wouldn't build anymore on FreeBSD, Android, Apple platforms and others who use that compiler. That would be way worse for your users than just migrating to a new macro for the subset of users impacted.

Respecting standard conventions is important for the ecosystem, and I would really encourage you to get started with this migration -- it's often easier than we think. We do a lot of these kinds of migrations in libc++ and it always works out even though it creates a bit of work for some folks -- that's unavoidable.

[DennisHeimbigner]

Ok, then let's do this in steps.

1. Add, say, NC_FillValue to netcdf.h
2. Make sure there is warning in our RELEASENOTES.md and elsewhere that calls out that _FillValue is deprecated in favor of NC_FillValue and that _FillValue will be removed as part of the next release (i.e. after the release that include step 1 and step 2.
3. Finally, remove _FillValue from netcdf.h.

I realize that this is slower than is desirable because it takes two released to finally remove _FillValue.
Of course, if everyone else decides to do it in one release, I am ok with that.

[ldionne]

Personally, I think this is a perfectly fine timeline and approach to do this. If we were deprecating something in libc++, that's how I would do it (and that's what we often do) -- it's normal to take some time to do these things.

[dopplershift]

I'm +0.5 on skipping deprecation and just documenting in release notes since:

1. We never should have had _FillValue--this violates even the C89 standard.
2. The result of the change is failure to compile, so you're not going to get incorrect runtime behavior
3. The fix is a quick search and replace (easily done in an editor or using sed)

With that said, I'm not on the hook to deal with support on this, so I completely defer.

[WardF]

I suspect we should be able to refactor this out, while leaving the name of attribute alone (as we'd expect). Tagging this for the upcoming release, it should (ideally) be pretty straightforward, but if it's not, we can reconsider our approach.