mpusz / mp-units

The quantities and units library for C++
https://mpusz.github.io/mp-units/
MIT License
997 stars 79 forks source link

Renaming of accessor functions #484

Closed mpusz closed 9 months ago

mpusz commented 10 months ago

Changes introduced by this PR:

Resolves #477 and relates to #479

mpusz commented 10 months ago

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

JohelEGP commented 10 months ago

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 (https://github.com/mpusz/mp-units/pull/484#pullrequestreview-1597181948).

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 commented 10 months ago

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 https://github.com/mpusz/mp-units/issues/477#issuecomment-1694730634.

mpusz commented 10 months ago

It still has any issue you might find with any other such standard interface, such as the one referenced by https://github.com/mpusz/mp-units/issues/477#issuecomment-1694730634.

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 commented 10 months ago

@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 commented 10 months ago

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

mpusz commented 9 months ago

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

chiphogg commented 9 months ago

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 commented 9 months ago

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.