Miscellaneous issues

[JohelEGP]

• detail::_dimension_t, detail::_unit_conversion and detail::_unit are documented to be used for RTTI testing. Perhaps you meant compile-time testing?
• Perhaps you should document in the README and the code documentation for doxygen that some parts of the code are licensed under a different license than LICENSE.
• Consider making the implementation of ratio_sqrt and the like more digestible by reimplementing the calculations done at the type level as constexpr calculations. Asserts like static_assert(value > 0, "Overflows when computing 2 * N"); can be replaced with assert(value > 0 && "Overflows when computing 2 * N"); (I'm not sure if this might be a set back to the #114 effort, so it'd be better to have that first). See https://stackoverflow.com/questions/3692954/add-custom-messages-in-assert. The resulting constexpr function(s) can then be used like this, for example:

  template <class R1, class R2>
  using ratio_gcd = std::ratio<std::gcd(R1::num, R2::num), std::lcm(R1::den, R2::den)>;

• convert has some !!boolean-expression, were the !! to work around bugs in previously supported compilers? Also, the std::is_same_v<std::ratio<0>, ...> might be better expressed with std::ratio_equal.
• Consider using the deprecated attribute for the value_type type aliases, like [[deprecated("Prefer underlying_type")]].
• There sure are many ways of converting a unit to and from other units, dimensionless and arithmetic types. This might be overwhelming to an user.
• Conversion to and from std::chrono::duration types is done by going through an explicit conversion to std::chrono::nanoseconds. Besides the possible lack of optimal code generation, maybe if only in non optimized builds, it might imply some loss of precision. I'm not sure, floating-point numbers can be weird. I think the conversions can be changed to work directly on the rep and period of std::chrono::duration, without the intermediary std::chrono::nanoseconds.
• Perhaps you meant private here. Although visibility doesn't affect friendship, maybe you meant to document that it's an implementation detail.
• inline is used in functions where it is implied by being both constexpr and a template. Is this a workaround from a previously supported compiler?
• Is it intentional that pow only supports positives powers? Maybe that should be documented.
• That's certainly not a namespace. Maybe this is a copy&paste error.
• Wikipedia says that the concentration units are not part of the SI, so this might be another copy&paste error. Maybe there are others?
• The constants are missing inline, and I think static is implied by being constexpr variables at namespace level.

[nholthaus]

Just going to leave some notes for myself here...

detail::_dimension_t, detail::_unit_conversion and detail::_unit are documented to be used for RTTI testing. Perhaps you meant compile-time testing?

YES

Perhaps you should document in the README and the code documentation for doxygen that some parts of the code are licensed under a different license than LICENSE.

Probably? I'm not much into the legalese, is there some other project you know of @johelegp that we could copy how they disclaim that?

Consider making the implementation of ratio_sqrt and the like more digestible by reimplementing the calculations done at the type level as constexpr calculations. Asserts like static_assert(value > 0, "Overflows when computing 2 * N"); can be replaced with assert(value > 0 && "Overflows when computing 2 * N"); (I'm not sure if this might be a set back to the #114 effort, so it'd be better to have that first). See https://stackoverflow.com/questions/3692954/add-custom-messages-in-assert. The resulting constexpr function(s) can then be used like this, for example:

template <class R1, class R2>
using ratio_gcd = std::ratio<std::gcd(R1::num, R2::num), std::lcm(R1::den, R2::den)>;

YES. This was another part of the code that I didn't write and can probably be made a lot simpler w/ c++17.

convert has some !!boolean-expression, were the !! to work around bugs in previously supported compilers? Also, the std::is_same_v<std::ratio<0>, ...> might be better expressed with std::ratio_equal.

Need to investigate this...

Consider using the deprecated attribute for the value_type type aliases, like [[deprecated("Prefer underlying_type")]].

So I originally only had underlying_type, but boost::units converts complained about the semantics. I didn't see the harm in being a little redundant so I appeased both crowds. I think deprecating one or the other would put me back in the same situation.

There sure are many ways of converting a unit to and from other units, dimensionless and arithmetic types. This might be overwhelming to an user.

yes. part of the problem of the evolution of the library and maintaining support for old compilers. I think this can be solved in documentation? My recommendation is to always take the implicit-conversion approach (for ease and readability) unless there's a good reason not to.

Conversion to and from std::chrono::duration types is done by going through an explicit conversion to std::chrono::nanoseconds. Besides the possible lack of optimal code generation, maybe if only in non optimized builds, it might imply some loss of precision. I'm not sure, floating-point numbers can be weird. I think the conversions can be changed to work directly on the rep and period of std::chrono::duration, without the intermediary std::chrono::nanoseconds.

I think I took the path of least resistance on that one. This is a good idea.

Perhaps you meant private here. Although visibility doesn't affect friendship, maybe you meant to document that it's an implementation detail.

you're right, it's a detail and could be private.

The constants are missing inline, and I think static is implied by being constexpr variables at namespace level.

Probably me being overly explicit again, and wrong about it. I'll look into this.

inline is used in functions where it is implied by being both constexpr and a template. Is this a workaround from a previously supported compiler?

Probably a hole in my knowledge, but in either case only c++17 compilers will be supported from 3.0 on, so it should get cleaned up.

Is it intentional that pow only supports positives powers? Maybe that should be documented.

There is a constexpr pow<> that only uses positive integers and a cmath-style pow() that can handle the normal args. pow<> used to be called cpow<>, so this probably needs to be disambiguated in the documentation.

That's certainly not a namespace. Maybe this is a copy&paste error.

yeah some type of refactoring issue for sure. Needs to be cleaned.

Wikipedia says that the concentration units are not part of the SI, so this might be another copy&paste error. Maybe there are others?

Result of an aggressive regex I think (defining all those units was really tedious...). I'll remove the SI reference.

[JohelEGP]

On the license, see https://github.com/nlohmann/json#license.

I think deprecating one or the other would put me back in the same situation.

I just assumed it was deprecated because the code says May be removed in future versions. Prefer underlying_type..

[JohelEGP]

Document that the unit containers are guaranteed to be trivial types. Some standard vocabulary types do that, like std::optional<T> if T is trivial. I guess some people might be interested in that.

[JohelEGP]

The above is a suggestion, but I worded it like an order. Sorry about that.

• Conversion to and from std::chrono::duration types is done by going through an explicit conversion to std::chrono::nanoseconds. Besides the possible lack of optimal code generation, maybe if only in non optimized builds, it might imply some loss of precision. I'm not sure, floating-point numbers can be weird. I think the conversions can be changed to work directly on the rep and period of std::chrono::duration, without the intermediary std::chrono::nanoseconds.

https://github.com/nholthaus/units/pull/126/commits/c59d6e9434fb6613e41e6fdeedd3ba646c494076 of #126 takes care of this.

[GElliott]

Probably a hole in my knowledge, but in either case only c++17 compilers will be supported from 3.0 on, so it should get cleaned up.

Are there must-have features in C++17 that you need for 3.0, or is this move motivated by code cleanliness? I've been looking forward to the mixed-types work in 3.0, but I'm afraid that I will need to stick with C++14. I work with a large shared code base that targets a variety of CPU architectures and compilers. Not all compilers (non-clang, non-mainline-gcc, non-msvc) are ready for C++17.

[nholthaus]

Code cleanliness and a lot of criticism from WG21 that it's not "modern" enough to act as a reference implementation for a future std library. I probably can't use 3.0 either for what it's worth.

You made most of the changes, could we back-port them to a 2.4? Un-refactoring things like convert will suck but are probably possible.

If not these figure themselves out pretty quickly. When I started the project we couldn't use constexpr because of compiler support issues.

[JohelEGP]

Code cleanliness and a lot of criticism from WG21 that it's not "modern" enough to act as a reference implementation for a future std library. I probably can't use 3.0 either for what it's worth.

Do you attend meetings or something?

You made most of the changes, could we back-port them to a 2.4? Un-refactoring things like convert will suck but are probably possible.

Sure. Is it somehow related to the above? By back-porting to a 2.4, I guess you mean to maintain backwards compatibility as required by semantic versioning (that's what you use, right? You should declare it (last paragraph.))

What kind of backwards compatibility are you looking for?

[nholthaus]

Are we ready to close this?

[JohelEGP]

Yeah, I think that's okay.

You made most of the changes, could we back-port them to a 2.4?

I'll look into this after my current PRs.

[nholthaus]

I think a lot of users would like a back-port since we're creating a pretty large barrier to upgrade with the new compiler requirements and renaming. I don't really think it's worth the effort though, and I'm (personally) not going to provide continuing support for v2 once we release v3, which a back-port kindof implies.