-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The behavior of raw data accessors #412
Comments
As mentioned elsewhere, I'm actually not a big fan for |
First, please let me thank you for a lot of great feedback you provided in the last few days! That helps a lot and shines the light on interesting problems. Thanks!!! As I just stated in #239, representation-type conversion may introduce data truncation. If quantity_of<isq::distance[m]> auto q = get();
auto num1 = q.number_in<q.unit>(); // give me what you have
auto num2 = q.number_in<mm>(); // convert in a safe direction
// auto num3 = q.number_in<km>(); // compile-time error if not a floating-point representation
auto num3 = quantity_cast<km>(q).number_in<km>(); // now OK Now the question is if we want such a feature and if it should replace the current |
I like that overload set you provided, with exactly the semantics you describe. To me, The fact that the third example requires a "double cast" is really a plus to me: I can get assurance that Given that |
That is right, but it should be considered an implementation detail rather than an official interface. This is the same as for I even consider renaming the member object to something like |
I agree. Because of |
OK, so the last thing to note here. |
If the number type is heavy, then we may care. Numbers usually shouldn't be, but who knows, there could be a use-case. So you convinced me. Let's keep it. |
I just realized one more thing. |
Ah, good point. As we seem to move towards preferring values over types (see boost::hana), the "unit specification" (cast specification restricted to units) that we supply could actually be a value rather than a type (though I don't remember off the top of my head if that is the case in current V2 for units). Then, |
Yes, it is. Users typically do not notice because of the fact that we name a type and an instance with the same identifier. To make it explicit: quantity_cast<quantity<isq::speed[km/h]>>(q); // takes type
quantity_cast<km/h>>(q); // takes value
quantity_cast<isq::speed>>(q); // takes value
quantity_cast<isq::speed[km/h]>>(q); // takes value
quantity_cast<int>(q); // takes type In general, units, quantity_spec, and references are always passed as NTTP objects. |
Yes, I like this idea! |
To summarize:
|
Done! :-) However, this might be a good place to discuss one more issue. Currently, Alternatively, we could return by values/copies for those, which, on the other hand, could be perceived as a broken overload set as sometimes you get a reference (for const lvalues) or an rvalue (for rvalues). To make it clear. Here is what we have now: And here is a possible alternative: [[nodiscard]] constexpr const rep& number() const& noexcept { return number_; }
[[nodiscard]] constexpr rep number() && noexcept { return std::move(number_); }
[[nodiscard]] constexpr rep number() const&& noexcept { return std::move(number_); } What is the right way to do here? |
I'm using this library in production, but I've avoided constructors and accessors entirely, wrapping everything in a pair of templated functions. Their impelmentations are icky, but the user-facing part is millimeter_t foo = fromCount<millimeter_t>(42.0); // 42.0 mm
fTakingMillimeters(getCount<millimeter_t>(foo)); // Calls fTakingMillimeters(42.0). Being strict about only using using Dist_mm = millimeter_t;
using Vol_mm3 = decltype(pow<3>(std::declval<Dist_mm>()));
using Vol_mL = millileter_t; then -Vol_mm3 vol;
+Vol_mL vol;
...
fTakingMM3(static_cast<double>(vol)); // The change to Vol_mL silently breaks this line. So instead I have -Vol_mm3 vol;
+Vol_mL vol;
...
fTakingMM3(getCount<Vol_mm3>(vol)); //< Compile error: vol is not a Vol_mm3. |
@BenFrantzDale, thanks for sharing! I do not believe that |
Yes. I guess it's the other way around. I'd encourage you to consider dropping the |
Related: right now getting the count of a percentage gives the scalar value. So if you add that, be sure that Closely related (maybe I should make a separate ticket): I think I could see |
Yeah, I also heard this in the ISO C++ Committee from one person. On the other hand, the rest of the room would like to have a traditional way of doing things (via constructors). |
I believe it, but so far, I've found that any holes in unit safety let bugs through. With a safe getter and factory function and literals, it's pretty darn solid. |
Please do |
In the V2 framework you will be able to do something like that |
I am not sure what you mean by that. Could you please provide more info? constexpr dimensionless<one> q{0.25};
static_assert(quantity_cast<percent>(q).number() == 25); |
@BenFrantzDale --- I think your getters/setters use the correct unit-safe pattern, but how do you prevent people from calling the constructors? |
An alternative would be to give the "direct access" version of the function a different name. In Au, our value-semantic copying accessor (to get the value of a quantity This means it's OK to do the following, for example: auto x = inches(3);
++(x.data_in(inches));
EXPECT_EQ(x, inches(4)); |
Stepping back, and giving the bigger picture on this issue: I can share some real-world implementation experience. When we were first writing Aurora's units library, folks were worried that it would be too burdensome to have to name the unit at the callsite. I said I'd consider adding a "just give me the underlying value!" function if I got a compelling use case. Since then, we have had years of experience with widespread use in production, and I have yet to hear a single complaint. The pattern works, and it makes code both safer and more readable. So I think it'll be great to remove all instances of By the way: mp-units is the very first units library I have seen (other than Au) which takes these unit-safe interfaces seriously. This is thrilling! I am heartily encouraged by this change. |
Am I correct that Aurora works only with C++ arithmetic (fundamental) types? Those are cheap to copy. For me, the most important reason to leave a raw accessor is to be able to access the underlying storage by const reference. It might be important if someone uses some untrivial representation type. |
Yep --- only arithmetic (fundamental) types for now. Expanding this is in-scope for us, however; we're tracking it in aurora-opensource/au#52. (BTW, nice work in mp-units on explicitly specifying a concept for the representation type!) |
Thanks, but still, we need to do a better job here. See: https://github.com/mpusz/units/blob/622b3e3cbd21506d53fba4d97204fac6ab108aef/src/core/include/mp_units/bits/quantity_concepts.h#L105-L108. |
Coming back to accessing raw data (I just asked myself how to do it in code using the library).
Consequently, removing the unit would be the reverse operation:
... which, when written on paper this way, would read aloud as "value of variable tencm in cm". Seeing it this way, I'd also intuitively expect:
Interestingly enough, I can already do something quite similar:
, then again
will not work. As already stated, the consequences this would have are completely in the mist for me, but I leave you with this food for thought: IMO code would be easily readable when it looks like this:
|
On the one hand I agree, and I do see the elegance of Food for thought. |
Expanding on @BenFrantzDale's comment: what is What's really interesting is that the notion of "value" becomes ambiguous for every dimensionless unit, except the "unit one". There are two perfectly reasonable candidates for "the" value of such a quantity. A consequence which follows is that rather than try to guess which one the user meant, this kind of implicit conversion should be forbidden, as I explained here: nholthaus/units#301 (comment) The upshot is that while the "dividing method" really is creative, elegant, and intuitively appealing, I don't think it works out in practice as an API... whereas |
Yes, this is exactly what I wanted to write here. The fact that the Also, to extend on the @chiphogg comment. mp-units provides implicit conversions for dimensionless quantities only if they are expressed in a unit |
Implicit conversions to/from built-in types appear to be a potential source of confusion for dimensionless units with a scaling factor (e.g., percent<>) [0]. Disallowing implicit conversions in those cases might be a not-terrible way to eliminate this potential for confusion without decreasing usability. The idea for limiting the implicit conversion to unscaled values was adapted from mpusz/units [1]. I'm not entirely confident this implementation is the best approach, but it's at least not obviously broken. [0]: nholthaus#301 [1]: mpusz/mp-units#412 (comment)
As @JohelEGP noticed in #411 (comment), current accessors remove all encapsulation. Should we remove lvalue overload so the only remaining would be:
The text was updated successfully, but these errors were encountered: