Closed ypearson closed 2 years ago
Possible workaround:
using threadPitch_unit_mm = struct SI::detail::unit_t<'L', std::ratio_multiply<std::ratio<-1L>, std::ratio<1L>>, int64_t, std::milli>;
constexpr threadPitch_unit_mm threadPitch_per_mm{2};
constexpr auto x = 2.0 / SI::milli_metre_t<double>{1}; //Works
constexpr auto x = 2 / SI::milli_metre_t<double>{1}; // Doesn't work
It seems the corresponding operator is missing from unit.h
Will add it.
Thank you! Appreciate it very much
On a closer look, you might have found a genuine bug/weird behavior of the implementation here, as the resulting value is divided by the ratio (which should not happen). To clarify, assuming the following calculation
int64_t v{1000}
const auto mm = 2_mm;
const auto r = v / mm;
The expected result would be of type mm^-1
and the value of it would be 500.
I'm glad you're seeing what I'm seeing. For my application I need 1/mm for describing a mechanical interface.
Do you think the workaround I posted is okay for right now?
Thanks for your attention to this.
On Fri, Feb 4, 2022, 3:07 AM Dominik Berner @.***> wrote:
On a closer look, you might have found a genuine bug/weird behavior of the implementation here, as the resulting value is . To clarify, assuming the following calculation
int64_t v{1000} const auto mm = 2_mm; const auto r = v / mm;
The expected result would be of type mm^-1 and the value of it would be 500.
— Reply to this email directly, view it on GitHub https://github.com/bernedom/SI/issues/101#issuecomment-1029743212, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADDTMLEZKQMFHVDOY3L3P2TUZOCNJANCNFSM5NKJ57RA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you authored the thread.Message ID: @.***>
Yes the workaround might work. But there might be a bug in the operator at the momeent, so use it with care and check the behavior first.
Okay thank you. This is highly appreciated, I'm using your library for some really important work. Keep me posted!:)
On Fri, Feb 4, 2022, 9:39 AM Dominik Berner @.***> wrote:
Yes the workaround might work. But there might be a bug in the operator at the momeent, so use it with care and check the behavior first.
— Reply to this email directly, view it on GitHub https://github.com/bernedom/SI/issues/101#issuecomment-1029994225, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADDTMLEDGZFQBPV72ZR37MLUZPJI5ANCNFSM5NKJ57RA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you authored the thread.Message ID: @.***>
The issue is resolved in this branch https://github.com/bernedom/SI/tree/bugfix/fix-compilation-failure-for-constexpr-operator and will be in release 2.4.1
Could you please check it quickly, if it works as you expect and give me feedback?
constexpr auto pitch = 2 / 1_mm; // works
constexpr auto x = 2.0 / SI::milli_metre_t<double>{1}; // works
constexpr auto x = 2 / SI::milli_metre_t<double>{1}; // works but will produce a compiler warning because of the narrowing from int to double
Absolutely, I'm traveling at the moment. But by next week I'll be at my desk looking at this carefully.
On Fri, Feb 4, 2022, 11:40 AM Dominik Berner @.***> wrote:
The issue is resolved in this branch https://github.com/bernedom/SI/tree/bugfix/fix-compilation-failure-for-constexpr-operator and will be in release 2.4.1
Could you please check it quickly, if it works as you expect and give me feedback?
constexpr auto pitch = 2 / 1_mm; // works constexpr auto x = 2.0 / SI::milli_metre_t
{1}; // works constexpr auto x = 2 / SI::milli_metre_t {1}; // works but will produce a compiler warning because of the narrowing from int to double — Reply to this email directly, view it on GitHub https://github.com/bernedom/SI/issues/101#issuecomment-1030103207, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADDTMLGLM6CI6GWKN6NYHULUZPXNBANCNFSM5NKJ57RA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you authored the thread.Message ID: @.***>
Back online, looking at the fix today
constexpr auto x0 = 2UL / 1_mm; // No narrowing warning here
constexpr auto x1 = 2L / 1_mm; // No narrowing warning here
constexpr auto x2 = 2 / 1_mm; // Narrowing warning here from ‘long int’ to ‘int’
constexpr auto x3 = 2.0 / SI::milli_metre_t<double>{1}; // works
constexpr auto x4 = 2L / SI::milli_metre_t<double>{1}; // Narrowing from long int to double
constexpr auto x5 = 2UL / SI::milli_metre_t<double>{1}; // Narrowing from long int to double
I can confirm this works with the warnings.
What is odd is 2
is type int
, why is there a narrowing from from long int to int
? I would think the 2
should be implicitly converted to a int64
and thus no narrowing.
The C++ standard only offers unsigned long long int
and long double
and various char_t
as arguments for user defined literals for. So for numerical literals it's either long long int
or long double
as type.
Since SI should support signed values choosing int64_t
(or long long int
on 64bit machines) as default storage type for integrals was a kind of natural choice. I thought about creating literals like _mmF
which would return a float
but then the number of literals would explode without too much benefit. There currently exists no way to templatize
In the current implementation of int / unit<int64_t>
will return a unit<int>
, but I'm planning to implement the standard arithmetic conversions as soon as I find the time as described here: https://en.cppreference.com/w/c/language/conversion (This will not be part of the current fix for the original problem)
In the end I try to keep as close as possible to the behavior as when calculating with raw numbers. More details about narrowing and possible loss of precision is described here: https://github.com/bernedom/SI/blob/main/doc/implementation-details.md#implicit-ratio-conversion--possible-loss-of-precision
As for the second calculation 1_mm/2
will do an integer division so 0 is what is expected. Automatically switching types would work against the strong type safety intended and also increase the calculation overhead way too much, because each time the result has to be checked and possibly calculated a second time.
Thanks for the response. Makes sense to me!
I just realized I can do 1.0_mm/2
to get the result I was looking for
include <SI/length.h>
using namespace SI::literals; constexpr auto pitch = 2 / 1_mm; // OK, without constexpr, but need it
Expected behavior I expected to pitch to be of type 2 mm exp -1 (2 per millimeter) ratio of -1
I/include/SI/detail/unit.h:474:33: error: ‘(2 / 0)’ is not a constant expression