V3.x rename conversion factors #211

nholthaus · 2018-11-05T22:31:12Z

No description provided.

…nit types in conversion factors.

…flect this.

…p for removing predefined conversion factors.

…cause we know that int dB units don't work

nholthaus · 2018-11-05T22:46:19Z

@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 · 2018-11-05T22:56:50Z

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

coveralls · 2018-11-05T22:58:51Z

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

nholthaus · 2018-11-05T23:06:03Z

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 · 2018-11-05T23:35:22Z

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.

JohelEGP · 2018-11-05T23:43:18Z

include/units/core.h

@@ -2508,8 +2511,8 @@ namespace units
 			std::enable_if_t<detail::is_time_conversion_factor<Cf> && detail::is_losslessly_convertible<Rep, T>, int> =
 				0>
 		constexpr unit(const std::chrono::duration<Rep, Period>& value) noexcept
-		  : linearized_value(NumericalScale::linearize(units::convert<unit>(
-				units::unit<units::conversion_factor<Period, dimension::time>, Rep>(value.count()))()))
+		  : linearized_value(NumericalScale::linearize(units::convert<unit>(units::unit<units::conversion_factor<Period, dimension::time>, Rep>(value.count()))


This line is longer than the Clang Format limit. Why don't you turn it on to run on every save?

it's on, I think visual studio's implemention isn't very good. I've seen it crap out a few times.

I've been looking into automating it in a Travis build. I'll get back to you when I'm successful.

nholthaus · 2018-11-05T23:52:34Z

fixes #159

JohelEGP · 2018-11-05T23:53:00Z

include/units/core.h

@@ -510,14 +513,14 @@ namespace units
 #define UNIT_ADD_DIMENSION_TRAIT(unitdimension) \
 	/** @ingroup	TypeTraits*/ \
 	/** @brief		`UnaryTypeTrait` for querying whether `T` represents a unit of unitdimension*/ \
-	/** @details	The base characteristic is a specialization of the template `std::bool_constant`. \
+	/** @details	The base characteristic is a specialization of the template `std::bool_constant`. \ \ \ \ \


I see what you mean WRT MSVC's Clang Fromat.

JohelEGP · 2018-11-05T23:56:43Z

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 · 2018-11-05T23:57:17Z

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

JohelEGP · 2018-11-05T23:58:32Z

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

nholthaus · 2018-11-06T00:01:41Z

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

JohelEGP · 2018-11-06T00:05:55Z

unitTests/main.cpp


-	static_assert(has_equivalent_conversion_factor(


I hadn't updated these tests to test unit too, and not only the strong types.

JohelEGP · 2018-11-06T00:20:57Z

unitTests/main.cpp


 	EXPECT_EQ((std::hash<dimensionless<double>>()(3.14)), std::hash<double>()(3.14));
-	EXPECT_EQ((std::hash<unit<dimensionless_unit, double>>()(3.14)), std::hash<double>()(3.14));
-	EXPECT_EQ((std::hash<dimensionless<int>>()(42)), (std::hash<unit<dimensionless_unit, int>>()(42)));
+	EXPECT_EQ((std::hash<dimensionless<double>>()(3.14)), std::hash<double>()(3.14));


Same here. You'll see that the line above is now equal to this one. And below, too, at the tests for the ordering operators.

JohelEGP · 2018-11-06T00:22:54Z

unitTests/main.cpp


-	const unit<meters, int> c_m(1);


Around here they were intentional, too.

JohelEGP · 2018-11-06T00:28:40Z

unitTests/main.cpp

+	foot<double> a_ft(3.28084);
+	second<double> a_sec(10.0);
+	const meter<int> d_m(1), e_m(2);
+	const std::common_type_t<meter<int>, foot<int>> j(d_m);


This is fine since the common type of units are tested above, but generally I don't think there were undesirable uses of unit left that weren't testing unit and not the strong types.

should I revert c8d935f, or would you rather patch up the tests?

Reverting seems the better option.

JohelEGP · 2018-11-06T01:07:44Z

include/units/core.h

@@ -2437,7 +2440,7 @@ namespace units
 	 *				- \ref constantUnits "constant units"
 	 */
 	template<class ConversionFactor, typename T = UNIT_LIB_DEFAULT_TYPE, class NumericalScale = linear_scale>
-	class unit : NumericalScale, units::detail::_unit
+	class unit : public ConversionFactor, private NumericalScale, units::detail::_unit


This private is redundant.

JohelEGP · 2018-11-06T01:09:16Z

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 · 2018-11-06T01:30:43Z

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 · 2018-11-06T01:43:04Z

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 · 2018-11-06T18:09:58Z

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

JohelEGP

Sorry this is late. I made some review comments, but didn't know I had to explicitly submit them until recently.

JohelEGP · 2018-11-06T18:18:22Z

include/units/core.h

-	UNIT_ADD(namespaceName, giga##nameSingular, giga##namePlural, G##abbreviation, giga<namePlural>) \
-	UNIT_ADD(namespaceName, tera##nameSingular, tera##namePlural, T##abbreviation, tera<namePlural>) \
-	UNIT_ADD(namespaceName, peta##nameSingular, peta##namePlural, P##abbreviation, peta<namePlural>)
+	UNIT_ADD(namespaceName, femto##nameSingular##, femto##namePlural, f##abbreviation, femto<nameSingular##_t>) \


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

Maybe it's this added ##.

JohelEGP · 2018-11-07T01:44:04Z

unitTests/main.cpp

-	EXPECT_TRUE((std::is_same_v<square_meter, traits::strong_t<squared<meter>>>));
+	EXPECT_TRUE((std::is_same_v<meter_conversion_factor,
+		traits::strong_t<conversion_factor<std::ratio<1>, dimension::length>>>));
+	EXPECT_TRUE((std::is_same_v<kilometer_conversion_factor, traits::strong_t<kilo<meter_conversion_factor>>>));


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

I checked, and meter_conversion_factor should be changed to meter_t.

JohelEGP · 2018-11-07T23:09:17Z

include/units/core.h

@@ -2508,8 +2525,9 @@ namespace units
 			std::enable_if_t<detail::is_time_conversion_factor<Cf> && detail::is_losslessly_convertible<Rep, T>, int> =
 				0>
 		constexpr unit(const std::chrono::duration<Rep, Period>& value) noexcept
-		  : linearized_value(NumericalScale::linearize(units::convert<unit>(
-				units::unit<units::conversion_factor<Period, dimension::time>, Rep>(value.count()))()))
+		  : linearized_value(NumericalScale::linearize(


This linearize call can be replaced by accessing the .linearized_value of its argument.

nholthaus · 2020-10-03T16:19:23Z

@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?

…/units into v3.x-rename-conversion-factors

JohelEGP · 2020-10-03T23:49:41Z

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 · 2020-10-03T23:57:25Z

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 · 2020-10-03T23:59:19Z

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 < ***@***.***> 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 <#211 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACOYOHYVKM2YY6Q4MQWXRTLSI62B7ANCNFSM4GB5RXCQ> .

nholthaus · 2020-10-04T00:00:29Z

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

…

On Sat, Oct 3, 2020 at 7:59 PM Nicolas Holthaus ***@***.***> 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 < ***@***.***> 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 > <#211 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACOYOHYVKM2YY6Q4MQWXRTLSI62B7ANCNFSM4GB5RXCQ> > . >

Nic Holthaus and others added 11 commits November 1, 2018 15:32

first pass at renaming stuff (nothing is renamed yet). Allow use of u…

c87cbbf

…nit types in conversion factors.

units are technically conversion factors now. Update unit tests to re…

569dc3e

…flect this.

remove conversion factor tags based on unit abbreviations

0c93438

remove nested conversion factors inside unit definitions. This is pre…

6a68185

…p for removing predefined conversion factors.

renaming conversion factor - WIP

daecdc2

finished renaming conversion factors

71b3ef1

remove _t from unit names

0a3e415

add unit_t syntax back in as a synonym for unit<double>

b2ffd47

use unit names (instead of definitions) where possible in the unit tests

c8d935f

add std traits back in for dB. remove integer dB subtraction tests be…

93905b6

…cause we know that int dB units don't work

Merge branch 'v3.x' into v3.x-rename-conversion-factors

3644a63

allow references to succeed in the is_dimension_unit traits

484a58b

remove operator() from unit

ea02ee4

JohelEGP reviewed Nov 5, 2018

View reviewed changes

run clang format

eb6a770

JohelEGP reviewed Nov 5, 2018

View reviewed changes

JohelEGP reviewed Nov 6, 2018

View reviewed changes

unitTests/main.cpp

const unit<meters, int> c_m(1);

Copy link

Contributor

JohelEGP Nov 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Around here they were intentional, too.

JohelEGP reviewed Nov 6, 2018

View reviewed changes

switch to plural form of unit names

e29f489

nholthaus added 2 commits November 6, 2018 13:11

remove redundant private

03eddab

expand concentration unit names

da590ee

JohelEGP mentioned this pull request Nov 7, 2018

Failing common type of floating-point min and ms units #215

Open

JohelEGP reviewed Apr 23, 2019

View reviewed changes

update .gitignore to ignore clion

fafe76a

nholthaus added 7 commits October 3, 2020 12:59

some small fixes

093bd2f

Merge branch 'v3.x-rename-conversion-factors' of github.com:nholthaus…

55d3a3b

…/units into v3.x-rename-conversion-factors

fixed the strong unit test (I think?)

923f468

changed the wrong thing

b785c4a

Merge branch 'v3.x' into v3.x-rename-conversion-factors

afca4e8

remove the last vestiges of _t syntax

03c094a

Merge branch 'v3.x-rename-conversion-factors' of github.com:nholthaus…

77ad618

…/units into v3.x-rename-conversion-factors

nholthaus added 5 commits October 3, 2020 20:03

use clang-6 for CI

092db82

use clang-7 for CI

b3bd12e

use clang-7 for CI (xenial)

01fb576

use clang-7 for CI (focal)

a2c2041

lastest gcc/clang on focal

93f3a3e

nholthaus merged commit f9e2eca into v3.x Oct 4, 2020

nholthaus deleted the v3.x-rename-conversion-factors branch October 4, 2020 00:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V3.x rename conversion factors #211

V3.x rename conversion factors #211

nholthaus commented Nov 5, 2018

nholthaus commented Nov 5, 2018

JohelEGP commented Nov 5, 2018

coveralls commented Nov 5, 2018 •

edited

nholthaus commented Nov 5, 2018 •

edited

JohelEGP commented Nov 5, 2018

JohelEGP Nov 5, 2018

nholthaus Nov 5, 2018

JohelEGP Nov 5, 2018

nholthaus commented Nov 5, 2018

JohelEGP Nov 5, 2018

JohelEGP commented Nov 5, 2018

nholthaus commented Nov 5, 2018

JohelEGP commented Nov 5, 2018

nholthaus commented Nov 6, 2018

JohelEGP Nov 6, 2018

JohelEGP Nov 6, 2018

JohelEGP Nov 6, 2018

JohelEGP Nov 6, 2018

nholthaus Nov 6, 2018

JohelEGP Nov 6, 2018

JohelEGP Nov 6, 2018

JohelEGP commented Nov 6, 2018

nholthaus commented Nov 6, 2018

JohelEGP commented Nov 6, 2018

nholthaus commented Nov 6, 2018

JohelEGP left a comment

JohelEGP Nov 6, 2018

JohelEGP Nov 7, 2018

JohelEGP Nov 7, 2018

nholthaus commented Oct 3, 2020

JohelEGP commented Oct 3, 2020

JohelEGP commented Oct 3, 2020

nholthaus commented Oct 3, 2020 via email

nholthaus commented Oct 4, 2020 via email

V3.x rename conversion factors #211

V3.x rename conversion factors #211

Conversation

nholthaus commented Nov 5, 2018

nholthaus commented Nov 5, 2018

JohelEGP commented Nov 5, 2018

coveralls commented Nov 5, 2018 • edited

nholthaus commented Nov 5, 2018 • edited

JohelEGP commented Nov 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nholthaus commented Nov 5, 2018

Choose a reason for hiding this comment

JohelEGP commented Nov 5, 2018

nholthaus commented Nov 5, 2018

JohelEGP commented Nov 5, 2018

nholthaus commented Nov 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohelEGP commented Nov 6, 2018

nholthaus commented Nov 6, 2018

JohelEGP commented Nov 6, 2018

nholthaus commented Nov 6, 2018

JohelEGP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nholthaus commented Oct 3, 2020

JohelEGP commented Oct 3, 2020

JohelEGP commented Oct 3, 2020

nholthaus commented Oct 3, 2020 via email

nholthaus commented Oct 4, 2020 via email

coveralls commented Nov 5, 2018 •

edited

nholthaus commented Nov 5, 2018 •

edited