Strengthen unit aliases

[JohelEGP]

Discussed in #138.

This is a hack to get a feeling of how it would be to make the units actual types and not aliases. This would allow us to use CTAD (which isn't looking hopeful for type aliases in C++20) and better error messages.

My observations:

• The library adds a lot of symbols. I've read about linkers having problems with over 2^16 symbols. This change adds many more.
• This is WIP. e646b14, being the hack it is, adds redundant declarations to get things to work. More symbols.
• Unfortunately, the way std::common_type and std::hash work require us to add more symbols (specializations) for each strong unit alias.
• The tests pass when I change uses of std::is_same_v for has_equivalent_unit_conversion.

While working on this, I'll see if I can reduce the amount of symbols the library produces. Making is_unit what is_derived_from_unit (added in this hack) is and taking into account everywhere of this new definition should help there.

Foreseeable future work includes preserving the strong unit alias whenever possible. As it is, it converts to its base ASAP, losing on the advantage of better error messages. And, of course, making the strong unit alises templated entities.

I just wanted to get this hack out. I'll make it a proper work later. As such, much of this message will be out-of-date and edited out.

[Morwenn]

It's somewhat off-topic, but if you want to reduce symbols and template instantiations, a first easy step would be to replace the use of std::conditional_t with the implementation suggested in this article which basically amounts to this:

template<bool>
struct conditional
{
    template<typename T, typename U>
    using type = T;
};

template<>
struct conditional<false>
{
    template<typename T, typename U>
    using type = U;
};

template<bool B, typename T, typename U>
using conditional_t = typename conditional<B>::template type<T, U>;

This implementation of conditional only produces template instantiations for true and false and everything else is aliases. According to the article it can even improve compile times. considering that std::conditional is used in a number of places in units.h, that would be an easy first step to mitigate the problem.

[JohelEGP]

I mean to reduce instantiations of types in this library, so no unnecessary work is done by design. I'd rather not reimplement std:: stuff unless the amount of symbols produced is actually proven to be problematic, like someone hitting the 2^16 limit mentioned above and that conditional_t implementation reducing the symbols by 1/4th.

[JohelEGP]

I have started adding some macros meant to resolve #133 and #138. I didn't edit the current macros so that I can test the new ones incrementally. If they work, I'll update the current ones accordingly and discard the new ones.

[nholthaus]

I may have merged this badly...

[JohelEGP]

This is a mess of rebases and manual merging conflict resolutions of all my concurrent PRs. Don't worry about it.

[JohelEGP]

I have removed all unrelated commits. I'll now start trying out the macros.

[JohelEGP]

Seems like VS2017 and Clang 5 are chocking on the lack of viable constructors or deduction guides. My Clang 6, Travis's GCC 7 and my GCC 8 all compile fine and pass the tests. I'd guess that Clang 6 and GCC are correct. I'll see if adding some redundant deduction guides makes things work.

[nholthaus]

I'd be OK w/ clang 6 and gcc 7 as minimum versions (seems fair for c++17), but would really like to continue to support VS 2017.

[JohelEGP]

dimensionless is now workable with CTAD. I've had to remove defaulting the Underlying template parameter of its definition to double as it interferes with CTAD. The only place where it could help is in a default constructor deduction guide, which I can add if requested.

[nholthaus]

yeah to keep the syntax consistent (and to be consistent w/ previous usage) I think it would be good to include the deduction guide. As a design goal, we shouldn't require template usage in end-user code for basic dimensional-analysis functionality.

I'm going to play with this over the weekend so I can get a feel for the changes. Looks good overall. Did you notice whether it helped with the error messages or not?

[JohelEGP]

I have yet to check on the produced error messages.

[JohelEGP]

I've rebased and cleaned up the commits.

Seems like VS2015 didn't work with the deduction guide for the default constructor.

Later, I hope to convert the length units from aliases to strong type aliases. Followed by all the other units.

[JohelEGP]

I'll revert renaming the units to reduce the scope of the PR.

Strong typing will make #126 (comment) possible, which will also help simplify the logic of other things I want to propose.

[nholthaus]

yeah, renaming should be a PR on its own. I've been meaning to solicit some input from you about that. It was part of my initial motivation for 3.0 but I'm wondering if we're making a useful change or breaking compatibility for no good reason. I'm kind of on the fence.

[JohelEGP]

VS2017's error seems to be SFINAE related. I hope I can decipher it.

[JohelEGP]

Looks like the decibel units of integral underlying type are too error prone:

units/unitTests/main.cpp:1577: Failure
      Expected: ++b_dBW
      Which is: 2 dBW
To be equal to: dBW_t(2)
      Which is: 0 dBW
units/unitTests/main.cpp:1578: Failure
      Expected: b_dBW++
      Which is: 2 dBW
To be equal to: dBW_t(2)
      Which is: 0 dBW
units/unitTests/main.cpp:1579: Failure
      Expected: b_dBW
      Which is: 3 dBW
To be equal to: dBW_t(3)
      Which is: 0 dBW
units/unitTests/main.cpp:1580: Failure
      Expected: +b_dBW
      Which is: 3 dBW
To be equal to: dBW_t(3)
      Which is: 0 dBW
units/unitTests/main.cpp:1581: Failure
      Expected: b_dBW
      Which is: 3 dBW
To be equal to: dBW_t(3)
      Which is: 0 dBW
[  FAILED  ] UnitContainer.unitTypeUnaryAddition (0 ms)
units/unitTests/main.cpp:1685: Failure
      Expected: --b_dBW
      Which is: 3 dBW
To be equal to: dBW_t(3)
      Which is: 0 dBW
units/unitTests/main.cpp:1686: Failure
      Expected: b_dBW--
      Which is: 3 dBW
To be equal to: dBW_t(3)
      Which is: 0 dBW
units/unitTests/main.cpp:1687: Failure
      Expected: b_dBW
      Which is: 2 dBW
To be equal to: dBW_t(2)
      Which is: 0 dBW
units/unitTests/main.cpp:1688: Failure
      Expected: -b_dBW
      Which is: -2 dBW
To be equal to: dBW_t(-2)
      Which is: 0 dBW
units/unitTests/main.cpp:1689: Failure
      Expected: b_dBW
      Which is: 2 dBW
To be equal to: dBW_t(2)
      Which is: 0 dBW
[  FAILED  ] UnitContainer.unitTypeUnarySubtraction (1 ms)

[JohelEGP]

/home/travis/build/nholthaus/units/unitTests/main.cpp:2476: Failure
      Expected: "8 cu_ft"
To be equal to: output.c_str()
      Which is: "0.226535 m^3"

This test is failing because pow<> doesn't preserve the strong type alias to the unit. This is currently only done in std::common_type in a few cases. I wonder how far we might want to go with this preservation.

[JohelEGP]

Now I'm trying to solve #126 (comment) and 69bb2fb.

[JohelEGP]

Another thing that could help with the error messages is making the unit_conversions themselves strong type aliases. Even units::unit<units::unit_conversion<... dim ... length ... std::ratio<1> ... std::ratio<0>, std::ratio<0>>, int>, which got shorter thanks to variadic dimensions, is less readable than units::unit<units::length::meter, int>. That's not too far from the units::length::meter_t<int> which this PR would offer (Note that the programmer would always write is as the latter, anyways). Perhaps it might even be preferable in the face of possible CTAD language support for alias templates and it seeming like a less invasive change in terms of LoC and symbols.

Then we could invest on making the unit manipulators result in strong type aliases themselves, like having units::divide<units::dimensionless_unit, units::time::seconds> be that rather than units::unit_conversion<... dim ... seconds ... std::ratio<-1> ... std::ratio<0>, std::ratio<0>> if units::frequency::hertz didn't exist.

I think this alternative, yet nonconflicting approach is worth investigating. Do you mind if I open a separate, possibly competing, PR for that?

[nholthaus]

Yeah I think it's worthwhile. There was a point in the early 2.0's where most of the (formerly named) units were still structs and we had error messages similar to `units::unit<units::length::meter, int> `. They were still very clear, even though they're more verbose than `units::length::meter_t<int>` (except for complex ephemeral units, which we don't need to worry about so much since they're basically user defined and domain specific, or intermediate). No one was really complaining about complex errors back then. Am I wrong in saying that this approach is not only non-conflicting, but actually complementary to the current PR? As far as I can tell, everything we strengthen is a net win.

[JohelEGP]

Yeah, they can also be complementary.

[JohelEGP]

Done.

[nholthaus]

looks like the brackets for the default constructors are only necessary for const instantiations... because a const must be initialized on creation. Non-const default constructed units don't need brackets, as expected. I'm going to look at function declarations next to see if there's any weirdness to that.

[JohelEGP]

Oh, of course. Thank you.

[JohelEGP]

Why a0bdbf5? They were supposed to test the default construction of const qualified units through CTAD.