ODR issues

[JohelEGP]

The fine-grained headers define partial specializations of traits::strong whose primary definition is in units/core.h. In https://github.com/nholthaus/units/pull/150#issuecomment-428280402 I outline what can be done about it. Modules should solve this, too.

[nholthaus]

Quoting you here so I can find this more easily:

I've thought about what could be done about the ODR issues.

1. Forward declare unit class templates and define strong specializations in <units/core.h>, like

   template<class Underlying>
   class square_meter_t;
   namespace traits
   {
   template<class Underlying>
   struct strong<conversion_factor<std::ratio<1>, units::dimension::area>>
   {
       using type = square_meter_t<Underlying>;
   };
   }

   Then, a user who #includes <units/length.h> and needs the return type of 1_m * 1_m to be complete gets an error saying that square_meter_t is undefined unless they also #included <units/area.h>.

2. Document preferring #includeing <units.h> over the finer-grained headers, and/or warn about the ODR issues when using them.

If doing 1., some compilation time would be lost compared to number 2, but it shouldn't be much compared to actually defining them. It shouldn't be time-consuming to implement, either: Take a snapshot of the macros defining units before they were scattered in different headers, apply some regexes to change, for example,

    UNIT_ADD(area, square_meter, square_meters, sq_m, conversion_factor<std::ratio<1>, units::dimension::area>)

to

    UNIT_DECLARE(area, square_meter, conversion_factor<std::ratio<1>, units::dimension::area>)

which becomes the first piece of code above, and finally browse the commits looking for renames and new units, and manually apply this new macro to them.

[nholthaus]

We should definitely take approach #1. My intuition is that we'd only need to declare 1 unit for each dimension (e.g. the SI or other canonical unit), but I'd have to test that.

[JohelEGP]

Unfortunately, only one unit declaration per dimension isn't enough. The problem's that the meaning of 1_m * 1_m can change depending on what you include (or don't) and their order. Unfortunately, I don't know how to exactly express the real problem.

[nholthaus]

ok so we forward declare them all. Still should compile a lot faster than before I think.

[JohelEGP]

Titus on ODR.

[JohelEGP]

It occurred to me that the units also specialize standard templates like std::common_type, so the solution suggested above is not enough. At least an error would be caught when attempting to use the result of 1_m * 1_m due to its type being incomplete. But when you don't need the complete type, you would get into ODR territory again. For example, std::common_type<decltype(1_m * 1_m), decltype(1_ft * 1_ft)> is ill-formed before the inclusion of <units/area.h>.

So I guess that all strong types for unit_conversions and units have to be forward declared in <units/core.h>, and all the specializations that UNIT_ADD_SPECIALIZATIONS touch be defined for those strong types in the same <units/core.h>. However, the std::hash::operator() should only be declared in <units/core.h> since its definition needs the complete type.

[nholthaus]

This is making me wonder whether splitting the headers is really the best approach. The original rationale was to improve compile time, and I think at minimum we'd need to run some benchmarks to see if the mass of declarations compromises that. Additionally having to define each new unit in two places is less than ideal.

I don't fully understand all the ins and outs of ODR, but it makes me wonder if we've broken ephemeral units:

The problem's that the meaning of 1_m * 1_m can change depending on what you include (or don't) and their order.

I think in v2.3 this wouldn't have been an issue because meters_squared was just an alias for what that type actually was, not the definition. Do ephemeral units still work right in all cases in v3 with strong types (say, assuming all definitions are put back in core.h)?

[JohelEGP]

This is making me wonder whether splitting the headers is really the best approach. The original rationale was to improve compile time, and I think at minimum we'd need to run some benchmarks to see if the mass of declarations compromises that. Additionally having to define each new unit in two places is less than ideal.

I agree.

Do ephemeral units still work right in all cases in v3 with strong types (say, assuming all definitions are put back in core.h)?

Definitely, just like before.

[JohelEGP]

This is making me wonder whether splitting the headers is really the best approach.

Maybe the problem are the strong units. With strong unit conversions, all that's really needed is forward-declaring them and defining their one specialization of traits::strong_t.

I really like the separation. I can finally work on <units/core.h> with QtCreator while enjoying all the good stuff an IDE has to offer without too much trouble.

[JohelEGP]

With strong units gone (and strong conversion factors still here), the simple directive of the comment above to forward declare the conversion factors and define their specialization of traits::strong should solve the ODR issues, I believe. It comes with repetition, potential to become unsynchronized, and a burden on program-defined units that want to avoid ODR issues (only when they define so many new units that they want to split them into various headers).