Skip to content
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

Default constructor and default value #273

Closed
StefanoD opened this issue Jul 26, 2024 · 2 comments · Fixed by #296
Closed

Default constructor and default value #273

StefanoD opened this issue Jul 26, 2024 · 2 comments · Fixed by #296
Labels
⬇️ affects: code (implementation) Affects implementation details of the code 📁 kind: cleanup Making the library nicer in some way, without affecting functionality much or at all 💪 effort: tiny A trivial change
Milestone

Comments

@StefanoD
Copy link

StefanoD commented Jul 26, 2024

Hi,
I just found a very shocking news in the documentation regarding default-constructor:

A default-constructed Quantity is initialized to some value, which helps avoid certain kinds of memory safety bugs. However, the value is contractually unspecified. You can of course look up that value by reading the source code, but we may change it in the future, and we would not consider this to be a breaking change. The only valid operation on a default-constructed Quantity is to assign to it later on.

This behaviour would differ from the underlying native type like double, int etc. which is contra-intuitive and thus error-prone!
We are using the default-constructor spread in our entire code base, with the assumption, that the default value equals to the default value of the native underlying type!
You cannot expect that users read every single line in your documentation. And even if I know this now, my collegues in my company still don't know this and it is hard to communicate this problem to every developer in a big company.
Documentating it, doesn't solve the problem, as I said: You must never expect, that user are reading carefully every single line of your documentation. I experience myself, that many collegues simply don't read my documentation and they expect, that everything works like they expect it intuitively.

@chiphogg
Copy link
Contributor

Sorry --- I didn't mean to give you a scare! 😅


First, on the risk of changing the value. While we do reserve the right to do that, I have no plans to actually change it, and I think it's overwhelmingly likely that we never will. (At least --- not to some other value. I'll explain below why we probably will change the manner of initialization.)

So why add this text?

The point is to encourage users to write code that makes their intent clear. Making a "default" quantity, and then using it in some computation, forces the reader to know the construction policy to know what the value is. And it forces the reader to guess at what the value was meant to be: did the author knowingly use the actual policy? Wrongly assume a different policy? Simply forget to initialize the variable? Initializing the variable eliminates these questions. And thanks to au::ZERO, it's also very easy to do.

(By the way: in the predecessor to this library, from Uber ATG, I did contractually specify that a default initialized quantity had a value of 0. Eventually, I realized that this policy did a poor job of expressing intent, and wasn't something we should encourage. It would have been hard to give it up in that library, because I hadn't yet invented ZERO, so setting a value of 0 was in some cases grotesquely verbose.)


Now, on the merits of the policy.

(Let's start with a huge caveat that initialization in C++ is infamously complicated, and I'm likely to make mistakes. That said, here's my best understanding.)

There are a couple of different ways that a default constructor is commonly invoked. First, there's value initialization:

// Value initialization:
int i{};
QuantityI<Meters> q{};

For value initialization, setting a nonzero value in Quantity() would indeed differ from the underlying types' behaviour. Currently, the underlying value gets explicitly initialized to 0 (as an implementation detail). So the initialization of q is analogous to i{0} rather than i{}. That's fine for native types, though, because value initialization is equivalent to zero initialization in this case.

(That said, looking at this with fresh eyes, I wonder whether we should just change value_{0} to value_{}. This would bring it in line with what you're asking, and might not even change any code. And, it would remove the assumption that the rep type is constructible from 0, and that the default for the rep type is semantically equivalent to 0.)

The other common way to invoke a default constructor is default initialization.

// Default initialization:
int i;
QuantityI<Meters> q;

Here, Quantity is unavoidably different from the underlying types, as any class would necessarily be. Default initialization invokes the default constructor for class types, but leaves native types uninitialized! Thus, invoking the default constructor in this way would be undefined behaviour for native types. For Quantity and other class types, we don't have UB, but it's still not a good pattern to default initialize a variable and then read from it.


So, in summary:

  • I think your suggestion that "the default Quantity constructor should default-construct the underlying rep" (by which I mean value-initialize it, using {}) would be more clearly correct than the {0} we're using now, and it wouldn't change any behaviour. I think we should make this change.
    • Once we make this change, we'll be fully consistent with native types for value initialization uses of the default constructor. (For default initialization uses of the default constructor, we could never be consistent.)
  • The point of the doc section is to discourage users from reading the value of a default-constructed Quantity. As a matter of style, I think it's important to always initialize variables to an explicit value, that makes the intent clear, before using them.

@chiphogg chiphogg added ⬇️ affects: code (implementation) Affects implementation details of the code 📁 kind: cleanup Making the library nicer in some way, without affecting functionality much or at all 💪 effort: tiny A trivial change labels Jul 26, 2024
@chiphogg chiphogg added this to the 0.3.6 milestone Jul 26, 2024
chiphogg added a commit that referenced this issue Sep 27, 2024
This won't change any actual values when the rep is a fundamental
arithmetic type, but it is more stylistically consistent with the idea
of a "default constructor".

We also update our contract to officially promise that we do this.  This
lets us get rid of the scary/threatening language that "we might change
this out from under you!", even though we never had any intention to
actually do so.  We retain the encouragement to use `au::ZERO`, and to
avoid using a default-constructed `Quantity` for anything other than
future assignment.

Fixes #273.
@chiphogg
Copy link
Contributor

@StefanoD, having considered this further, I've decided to remove the scary language from the documentation. I think that "default constructed Quantity contains default constructed rep" is a perfectly fine contract for us to promise, and we will do so as soon as #296 lands. I moved the stylistic guidance into a "warning" box, and there's no longer any hint or threat that we might change the behavior.

Thanks very much for giving me the invaluable perspective of how the documentation reads to an external user! I've learned from this that I need to be more careful about how I communicate my ideas, and I'll try to do that in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⬇️ affects: code (implementation) Affects implementation details of the code 📁 kind: cleanup Making the library nicer in some way, without affecting functionality much or at all 💪 effort: tiny A trivial change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants