weak namespace and include guards

[steven-varga]

Currently the OpenEXR Half Float implementation uses less expressive include guard _HALF_H_; makeing a slight modification ie: OPENEXR_HALF_H would allow better integration with other distantly related software packages such as H5CPP a general persistence library for modern C++.

#ifndef _HALF_H_
#define _HALF_H_
...
#endif

And a possible more descriptive, safer alternative:

#ifndef OPENEXR_HALF_H
#define OPENEXR_HALF_H
...
#endif

The suggested change would allow automatic detection of the Float16 header inclusion, providing a better experience to some of your users.

In addition the the include guards, the class half { ... } appears without namespace enclosure; would it be possible to place the class (or the better, all of them) within a namespace? example:

namespace oexr {
....
}

Background: In this HDF5 mailing list post Dr Werner Benger suggested to add OpenEXR/Float16 support to HDF5 C++ or H5CPP. Currently this support has been added with explicit configuration option -DWITH_OPENEXR_HALF

best wishes: steven

[lgritz]

I think the modern way of guarding against double inclusion is to eschew the practice of surrounding the entire header contents with a preprocessor test, and simply putting this at the start of the file:

#pragma once

We may still want a #define OPENEXR_HALF_H so that other header files can detect whether or not half is defined. (Accounting for lack of adequate reflection in C++.) An example of this can be seen here in the OpenImageIO project: https://github.com/OpenImageIO/oiio/blob/master/src/include/OpenImageIO/fmath.h#L816

That example also shows how eliminating or renaming the _HALF_H_ definition can break downstream software that has come to rely on its presence. That may be worth the tradeoff (I agree that _HALF_H_ is an unfortunate choice and too generic), but it's something we'll need to roll out carefully and only for a release that we all agree is allowed to be a compatibility break.

Similarly, while it would have been wise if from the start this half type was inside a project namespace, doing so now will certainly break a lot of downstream software. Again, that doesn't mean we shouldn't do so at some point, but it is going to be a painful change for everybody. Perhaps we would want to stage it, for example by having one release where we say:

namespace Imath {
    class half { ... };
}
using Imath::half;

which moves it inside the namespace, breaking link compatibility but preserving source compatibility. During that release we can educate and encourage all downstream software to change to "Imath::half" wherever they need it. Then in a subsequent major release, we can make the source-compatibility-breaking change of removing the using.

[steven-varga]

Thank you very much for looking into it! You have good points, I suspected the same and will re-post your answer. As for #pragma once vs. include guards: the latter provides means to detect your library in a header only settings.

best wishes: steven

[lgritz]

Reopening. I was not trying to dismiss or even delay your issue, just trying to get all the pros and cons on the table. These are good points and I want to keep the issue alive so we can make some improvements here when we're in a position to do a breaking release.

[meshula]

https://github.com/AcademySoftwareFoundation/openvdb/pull/927