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

Renaming of accessor functions #484

Merged
merged 22 commits into from
Sep 18, 2023
Merged

Renaming of accessor functions #484

merged 22 commits into from
Sep 18, 2023

Conversation

mpusz
Copy link
Owner

@mpusz mpusz commented Aug 27, 2023

Changes introduced by this PR:

  • quantity::numerical_value() refactored to quantity::numerical_value_ref_in(Unit)
  • quantity_point::quantity_from_origin refactored to quantity_point::quantity_ref_from(PointOrigin)
  • quantity and quantity_point compound assignment, pre-increment, and pre-decrement operators now preserve value category
  • quantity::numerical_value_ref_in(Unit) allowed only for lvalues
  • quantity_point::quantity_ref_from(PointOrigin) allows only for lvalues
  • quantity_point::quantity_from(PointOrigin) added
  • quantity::force_numerical_value_in(Unit) added
  • quantity::force_in(Unit) and quantity_point::force_in(Unit) added
  • usage of the above features in framework, examples, and unit tests

Resolves #477 and relates to #479

@mpusz
Copy link
Owner Author

mpusz commented Aug 27, 2023

Please review those changes and let's decide if this is what we meant in #477 and #479.

@mpusz mpusz changed the title Mpusz/issue477 Renaming of accessor functions Aug 27, 2023
Copy link
Collaborator

@JohelEGP JohelEGP left a comment

Choose a reason for hiding this comment

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

Here are my thoughts.

  • Requiring an interface like q.value_in(m)
    solves a real engineering (programming integrated over time) issue.

  • Library implementation and tests are unfortunately verbose,
    having gone from q.number() to q.value_ref_in(q.unit).

  • I'm thinking we should only have value_ref_in (and not also value_in).
    We explicitly need to use value_ref_in rather than value_in
    to not incur a copy after a value_cast or when we know q is in the correct type.
    Unfortunately, most of the time (all the time, for all I could see in this PR)
    we don't actually need the result to be a reference to the underlying number.

    I can see someone getting an error because they used the wrong reference, which is a good thing.
    I can also see them dropping the _ref because using value_in will result in correct code anyways, albeit less efficient.

@JohelEGP
Copy link
Collaborator

Consider another possible point against having both value_ref_in and value_in.
It is a combination of the last two items of my list of thoughts above (#484 (review)).

An unwary user would not only use value_in where they meant value_ref_in.
They also risk passing the wrong reference.
There are some examples in this PR of functions with a pair of quantity parameters.
An user could write q1.value_in(R2) and q2.value_in(R1)
where they meant q1.value_in(R1) and q2.value_in(R2).
If we instead had value_ref_in only (and not also value_in),
such formulations are always correct and efficient.

@JohelEGP
Copy link
Collaborator

such formulations are always correct and efficient.

Of course, I don't mean magically correct.
It still has any issue you might find with any other such standard interface,
such as the one referenced by #477 (comment).

@mpusz
Copy link
Owner Author

mpusz commented Aug 27, 2023

It still has any issue you might find with any other such standard interface,
such as the one referenced by #477 (comment).

We can change this behavior to return-by-value for rvalue-qualified overloads. However, in such a case value_ref_in will have to be renamed to not imply the ref part.

@mpusz
Copy link
Owner Author

mpusz commented Aug 27, 2023

@JohelEGP, thanks for the feedback! What do you think about the quantity_ref_from? It has some similar issues to value_ref_in.

I am also interested to hear what others think of those changes. Should we proceed, refactor, or maybe abandon them?

@JohelEGP
Copy link
Collaborator

Of course, the same applies to the other interfaces that do the same as the (value_in, value_ref_in) pair.

@mpusz
Copy link
Owner Author

mpusz commented Sep 13, 2023

It was lots of work but I think we are done now. Do you have any feedback before I merge it?

@chiphogg
Copy link
Collaborator

It was lots of work but I think we are done now. Do you have any feedback before I merge it?

I think the summary is out of date. It'd be good to update it so that it summarizes the API changes that are landing.

Also, it looks like we're still using the "force" word, even though we think it's confusing. Did you end up deciding it was the least-bad option?

@mpusz
Copy link
Owner Author

mpusz commented Sep 14, 2023

I think the summary is out of date. It'd be good to update it so that it summarizes the API changes that are landing.

I've updated the summary.

Also, it looks like we're still using the "force" word, even though we think it's confusing. Did you end up deciding it was the least-bad option?

I do not have a better alternative as I do not like coerce 😉 If we do not have a better idea now, I suggest merging it as it is and possibly renaming it in the future if we find a better alternative.

@mpusz mpusz merged commit becca90 into master Sep 18, 2023
35 checks passed
@mpusz mpusz deleted the mpusz/issue477 branch September 18, 2023 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to name a number accessor in quantity?
3 participants