search

nholthaus / units

a compile-time, header-only, dimensional analysis and unit conversion library built on c++14 with no dependencies.
http://nholthaus.github.io/units/
MIT License
947 stars 135 forks source link

V3.x rename conversion factors #211

Closed nholthaus closed 3 years ago

nholthaus commented 5 years ago

@johelegp meant to ask you about this:

[  FAILED  ] TypeTraits.is_data_unit (0 ms)
[ RUN      ] TypeTraits.is_data_transfer_rate_unit
/home/travis/build/nholthaus/units/unitTests/main.cpp:603: Failure
Value of: (traits::is_data_transfer_rate_unit_v<const gigabytes_per_second<double>&>)
  Actual: false
Expected: true

Why don't we want const references to pass the dimension trait?

JohelEGP commented 5 years ago

For the same reason we don't want it working on pointers. They're pointer/references to types, not the types themselves.

coveralls commented 5 years ago

Coverage Status

Coverage decreased (-100.0%) to 0.0% when pulling 93f3a3e870fbdd82e9386bdebf8e8b58573edc37 on v3.x-rename-conversion-factors into 65b00fd866f642a641bd2bb3169f0a419fbbe03b on v3.x.

nholthaus commented 5 years ago

I feel like this is one of those "c++ defaults are backwards" situations. The reason for the existence of dimension traits is for sfinae/concepts, where you may save a conversion over defaulting to common_type, etc. So all I really want to know is, "is this the dimension of unit that I want". 99% of the time const/ref/ptr doesn't matter for the concept to be valid (at least in my usage), so it seems better to check that in addition the 1% of the time than have to wrap most calls to the trait with remove_ref.

JohelEGP commented 5 years ago

That's alright, but it becomes a gotcha where it appears to work like the standard's design. Note all the other occurrences of tests in #208 that need to be updated after 484a58b.

nholthaus commented 5 years ago

fixes #159

JohelEGP commented 5 years ago

fixes #159

I'm not sure if it works with you even though it's disabled, but that needs to go in the first comment.

That said, aren't you renaming the units to their plural spelling?

nholthaus commented 5 years ago

no I was planning on using the singular form. Do you have a preference?

JohelEGP commented 5 years ago

Well, I thought the natural form in english was the plural spelling. Just like std::chrono::seconds.

nholthaus commented 5 years ago

yeah good point. It's just a more difficult refactor, but it shouldn't be a problem

JohelEGP commented 5 years ago

I don't think I have anything else to share. Reviewing this allowed me to notice a few things, as stated in the new issues.

nholthaus commented 5 years ago

I've been looking at the revert and actually I don't think are probably too many more places than what you pointed out where we shouldn't be using strong unit types. I like to keep the tests close to the intended usage of the syntax, and have 'bootstrapped' them a few times. We messed up the meanings of a few tests, but I'm fully confident that we didn't break any functionality.

let's fix the few we need to after the name refactor.

JohelEGP commented 5 years ago

Note that if you change the name of the units to their plural spelling, parts of my opinion in #212 will be applied due to how the macros are used in <units/concentration.h>. And that the singular name part of the UNIT_ADD macro will become essentially unused, I believe.

nholthaus commented 5 years ago

@johelegp the latest push fails one of the strong tests, but I'm not sure if something is wrong, or if the test is no longer valid because of how we define unit conversions in terms of other units in this branch. May want to take a look at the refactored METRIC_PREFIXES macro.

nholthaus commented 3 years ago

@johelegp I re-tried this branch with the gcc 10.1 and the strong_t alias template still gives us trouble. Could we refactor this in such way that strong_t is also a struct instead of an alias template?

JohelEGP commented 3 years ago

I'm back online. I'll try compiling it with GCC 10.2. Can you give me a github reference as to what's wrong with strong_t? Besides my issue to revert it. It's been a long while, after all, so I need the context.

JohelEGP commented 3 years ago

GCC 10.2 compiled it successfully and only a locale test failed:

...
1: /home/johel/Documents/C++/Forks/units/unitTests/main.cpp:2524: Failure
1:       Expected: "de_DE.utf8"
1: To be equal to: setlocale(6, "de_DE.utf8")
1:       Which is: NULL
1: /home/johel/Documents/C++/Forks/units/unitTests/main.cpp:2529: Failure
1:       Expected: point_de
1:       Which is: '.' (46, 0x2E)
1: To be equal to: ','
1:       Which is: ',' (44, 0x2C)
1: /home/johel/Documents/C++/Forks/units/unitTests/main.cpp:2535: Failure
1:       Expected: "2,5 km"
1: To be equal to: units::length::to_string(de).c_str()
1:       Which is: "2.5 km"
...
1: [  FAILED  ] UnitType.to_string_locale
...
nholthaus commented 3 years ago

sorry, I think I figured it out looking at the generated macro code, things were confusing being in the middle of the rename.

We should figure out if we actually need to revert strong types or not though. I don't really remember what we thought was wrong, and things seem to be working on the surface.

On Sat, Oct 3, 2020 at 7:49 PM Johel Ernesto Guerrero Peña < notifications@github.com> wrote:

I'm back online. I'll try compiling it with GCC 10.2. Can you give me a github reference as to what's wrong with strong_t? Besides my issue to revert it. It's been a long while, after all, so I need the context.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nholthaus/units/pull/211#issuecomment-703178380, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACOYOHYVKM2YY6Q4MQWXRTLSI62B7ANCNFSM4GB5RXCQ .

nholthaus commented 3 years ago

that's fine, the locale just isn't installed

On Sat, Oct 3, 2020 at 7:59 PM Nicolas Holthaus nholthaus@gmail.com wrote:

sorry, I think I figured it out looking at the generated macro code, things were confusing being in the middle of the rename.

We should figure out if we actually need to revert strong types or not though. I don't really remember what we thought was wrong, and things seem to be working on the surface.

On Sat, Oct 3, 2020 at 7:49 PM Johel Ernesto Guerrero Peña < notifications@github.com> wrote:

I'm back online. I'll try compiling it with GCC 10.2. Can you give me a github reference as to what's wrong with strong_t? Besides my issue to revert it. It's been a long while, after all, so I need the context.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nholthaus/units/pull/211#issuecomment-703178380, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACOYOHYVKM2YY6Q4MQWXRTLSI62B7ANCNFSM4GB5RXCQ .