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 https://github.com/nholthaus/units/pull/126#issuecomment-389393398 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 https://github.com/nholthaus/units/pull/126#issuecomment-389393398 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]

I've been thinking of splitting these changes in at least three, in no particular order:

1. Making the _t suffixed units parametrized over the underlying type.
2. Making the _t suffixed units strong type aliases.
3. Adding explicit support for CTAD.

By separating 2. and 3., the tests would have to explicitly state the underlying type template argument. That prevents running into the CTAD bugs of compilers with incomplete C++17 support. Then, we could add tests for explicitly testing CTAD.

[JohelEGP]

The reason for not using CTAD in the tests not testing CTAD, as mentioned above, is to avoid contributors having to consider when they're able to use it, since the tests they add and edit will go through compilers with incomplete C++17 support. That's fine, since even today, the tests aren't written like a user would. Indeed, many places use meter_t(1) instead of the more natural 1_m (which I'd expect would be more used than the former).

[nholthaus]

That's fine w/ me. The tests were originally written with VS 2013 support in mind. As long as the coverage is good, I don't think it matters.

Probably a good idea to include a CTAD test wrapped in an #ifdef which can be enabled from cmake.

On Thu, Aug 9, 2018 at 6:25 PM, Johel Ernesto Guerrero Peña < notifications@github.com> wrote:

The reason for not using CTAD in the tests not testing CTAD, as mentioned above, is to avoid contributors having to consider when they're able to use it, since the tests they add and edit will go through compilers with incomplete C++17 support. That's fine, since even today, the tests aren't written like a user would. Indeed, many places use meter_t(1) instead of the more natural 1_m (which I'd expect would be more used than the former).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nholthaus/units/pull/150#issuecomment-411917622, or mute the thread https://github.com/notifications/unsubscribe-auth/AJ2HHwIj-GWVJW4r3fxITnKdKJchryziks5uPLbggaJpZM4VGNsF .

[nholthaus]

I was looking back over the comments here and can't really tell what state the PR is in. Once we pull in the strong conversions, we should chat about what if anything is still outstanding.

Once we get this in and the renaming in, we're only some docs and a couple small issues away from a 3.0 build I think.

[JohelEGP]

The state is that it does too many things, so I've been splitting them into other PRs. Soon, it should only be about making the units strong type aliases, as the title suggests.

Once we get this in and the renaming in, we're only some docs and a couple small issues away from a 3.0 build I think.

I agree with that.

[JohelEGP]

I think I'm almost there with this PR. I should be able to include the CTAD support, as with the divide and conquer of PRs, it should now only require the same few deduction guides and test cases.

[JohelEGP]

I can't catch what VS2017 is complaining about.

Anyways, all that's missing is testing CTAD and maybe doing something about the ODR issues.

I had to include some hand-crafted specializations when dealing with the dimensionless units, as using the respective macros would have appended _t to their singularName. They should be able to substituted when the macros are reworked and the units renamed to their natural spelling.

[JohelEGP]

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.

How do you think we should move forward?

[JohelEGP]

This PR is ready.

[nholthaus]

It looked OK in a quick-and-dirty test but I may not have run into those situations. Can you provide examples before we decide?

Also I did the test with Visual Studio so maybe it's letting me do illegal things (which it sometimes does)

[JohelEGP]

You mean about this: https://github.com/nholthaus/units/pull/150#discussion_r228650793? I might be able to excavate some refs.

[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.