feat: unit constants #184

JohelEGP · 2020-12-25T23:11:34Z

Resolves #160.
Resolves #48.

The tests handles all comments raised in #48.

JohelEGP · 2020-12-26T01:56:00Z

namespace unit_constants isn't inline to avoid shadowing warnings and errors.

mpusz

I really like this approach. Thanks!

However, in order to be able to merge it, CI issues have to be resolved first.

src/include/units/physical/si/us/base/length.h

src/include/units/bits/one_rep.h

test/unit_test/static/unit_constants.cpp

JohelEGP · 2020-12-26T19:24:42Z

I really like this approach. Thanks!

However, in order to be able to merge it, CI issues have to be resolved first.

Thank you. ~~<snip>~~ It seems done.

JohelEGP · 2020-12-27T04:11:29Z

Here's the time taken by the main step of the CI.

Compiler + mode	`ef0c616`	`f052b1a`	`fa928a5`	`791d1ef`	`89359ca`	`2a522e3`	`master` `cabd8b0`	`master` `0fb821a`
MSVC Release	6m 3s	5m 40s	7m 59s	6m 33s		6m 36s	6m 35s	5m 54s
MSVC Debug			5m 55s			5m 3s	4m 41s	5m 29s
GCC 10.1 Release	4m 34s	4m 0s	4m 21s	4m 14s	4m 7s	4m 34s	3m 24s	3m 15s
GCC 10.1 Debug	4m 7s	4m 13s	3m 39s	3m 33s	3m 10s	3m 27s	2m 51s	3m 43s
GCC 10.2 Release	4m 14s	3m 52s	4m 19s	3m 31s	3m 54s	3m 23s	4m 5s	3m 6s
GCC 10.2 Debug	4m 19s	4m 18s	4m 0s	3m 27s	3m 17s	2m 58s	3m 19s	2m 39s

mpusz

Thank you for all those massive changes. A lot of good work!

I have two general comments:

When we leave some code commented out it would be good to provide a comment on why it is commented out.
Please use TODO comments if we should return to some lines of code in the future

and one question:

Do you think that we need *_per_* constants? It is good to have them for now to experiment, but I think that it might be better to just use * / * syntax which probably is easier to understand? It also allows fewer values to maintain and potentially standardize in the future. What is your experience here?

mpusz · 2020-12-27T09:23:53Z

src/include/units/bits/one_rep.h

+    return lhs;
+  }
+  template<QuantityValue Rep>
+  [[nodiscard]] friend constexpr Rep operator*(one_rep, const Rep& rhs) // pending #185


Please add TODO in a comment so it will be easier to find and resolve.
BTW, I really recommend Todo Tree VS Code extension.

mpusz · 2020-12-27T09:25:16Z

src/include/units/bits/one_rep.h

+  template<QuantityValue Rep>
+  [[nodiscard]] constexpr operator Rep() const noexcept
+  {
+    return quantity_values<Rep>::one();


src/include/units/physical/si/typographic/base/length.h

src/include/units/physical/si/fps/base/length.h

src/include/units/physical/si/fps/derived/speed.h

src/include/units/physical/si/iau/base/length.h

src/include/units/physical/si/international/base/length.h

src/include/units/physical/si/us/base/length.h

test/unit_test/static/unit_constants.cpp

mpusz · 2020-12-27T10:01:53Z

MSVC Debug seems hung up.

This seems to happen when an assert() is hit for VS Debug. Unfortunately, the assert message is not provided :-(

mpusz · 2020-12-27T10:09:43Z

Here's the time taken by the main step of the CI.

I do not think we should be concerned about it. This library is designed with C++20 modules in mind. Everything we will predefine in the library will be compiled once and will speed up the compilation of the users' code later on.

Also, now when I fixed VS compilation again and provided a CI for it, I plan to play a bit with vcperf to analyze and possibly improve the build times. Another round of compile-time optimization will come when clang will finally catch up...

mpusz · 2020-12-27T11:15:04Z

test/unit_test/static/unit_constants.cpp

+
+static_assert(2 * m == 2_q_m);
+static_assert(2 * s == 2_q_s);
+#if !defined(_MSC_VER)


Could you please use COMP_MSVC?

BTW, if you feel we can do better here, we can think about an alternative/better way to refer to compilers in a unified way...

For starters, all kind of global identifiers should be "namespaced". In this case, they should be prefixed with UNITS_ or MP_UNITS_ (just like the CMake options).

JohelEGP · 2020-12-27T18:07:41Z

~~<snip>~~

3. Do you think that we need `*_per_*` constants? It is good to have them for now to experiment, but I think that it might be better to just use `* / *` syntax which probably is easier to understand? It also allows fewer values to maintain and potentially standardize in the future. What is your experience here?

They should definitely go away. The purpose is to have natural syntax. 1 * m / s is so much better than 1 * m_per_s. I'm personally drawing the line at _per_. I welcome exceptions like mph, but I'm not so sure about units like Nm (https://en.wikipedia.org/wiki/Newton-metre), which Wikipedia shows as having "symbol N⋅m[1] or N m[1]". Considering that W_per_m_K will go away and there's no mK or m_K, so that one would have to write W / (m * K), should we keep unit constants like Nm?

mpusz

Thank you!!!

JohelEGP force-pushed the unit_constants branch from d98ff74 to 8e316ea Compare December 25, 2020 23:21

This comment has been minimized.

Sign in to view

JohelEGP force-pushed the unit_constants branch 7 times, most recently from bc67845 to 95e1c52 Compare December 26, 2020 01:48

JohelEGP force-pushed the unit_constants branch 4 times, most recently from 0f646bb to 8133fba Compare December 26, 2020 02:43

This comment has been minimized.

Sign in to view

mpusz requested changes Dec 26, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

JohelEGP force-pushed the unit_constants branch 3 times, most recently from 435b3fe to ef0c616 Compare December 26, 2020 23:24

This comment has been minimized.

Sign in to view

JohelEGP force-pushed the unit_constants branch from ef0c616 to f052b1a Compare December 27, 2020 04:15

mpusz requested changes Dec 27, 2020

View reviewed changes

JohelEGP force-pushed the unit_constants branch 2 times, most recently from 791d1ef to 89359ca Compare December 27, 2020 19:36

feat: unit_constants

2a522e3

JohelEGP force-pushed the unit_constants branch from 89359ca to 2a522e3 Compare December 27, 2020 19:52

mpusz approved these changes Dec 28, 2020

View reviewed changes

mpusz merged commit 8dede0d into mpusz:master Dec 28, 2020

JohelEGP deleted the unit_constants branch December 28, 2020 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: unit constants #184

feat: unit constants #184

JohelEGP commented Dec 25, 2020 •

edited

Loading

This comment has been minimized.

JohelEGP commented Dec 26, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mpusz left a comment

JohelEGP commented Dec 26, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

JohelEGP commented Dec 27, 2020 •

edited

Loading

mpusz left a comment

mpusz Dec 27, 2020

mpusz Dec 27, 2020

mpusz commented Dec 27, 2020

mpusz commented Dec 27, 2020 •

edited

Loading

mpusz Dec 27, 2020

JohelEGP Dec 27, 2020

JohelEGP commented Dec 27, 2020 •

edited

Loading

mpusz left a comment

feat: unit constants #184

feat: unit constants #184

Conversation

JohelEGP commented Dec 25, 2020 • edited Loading

This comment has been minimized.

JohelEGP commented Dec 26, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mpusz left a comment

Choose a reason for hiding this comment

JohelEGP commented Dec 26, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

JohelEGP commented Dec 27, 2020 • edited Loading

mpusz left a comment

Choose a reason for hiding this comment

mpusz Dec 27, 2020

Choose a reason for hiding this comment

mpusz Dec 27, 2020

Choose a reason for hiding this comment

mpusz commented Dec 27, 2020

mpusz commented Dec 27, 2020 • edited Loading

mpusz Dec 27, 2020

Choose a reason for hiding this comment

JohelEGP Dec 27, 2020

Choose a reason for hiding this comment

JohelEGP commented Dec 27, 2020 • edited Loading

mpusz left a comment

Choose a reason for hiding this comment

JohelEGP commented Dec 25, 2020 •

edited

Loading

JohelEGP commented Dec 26, 2020 •

edited

Loading

JohelEGP commented Dec 26, 2020 •

edited

Loading

JohelEGP commented Dec 27, 2020 •

edited

Loading

mpusz commented Dec 27, 2020 •

edited

Loading

JohelEGP commented Dec 27, 2020 •

edited

Loading