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

Explain freedoms available in F.16-F.18 #1412

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

russellmcc
Copy link
Contributor

As I discussed in #1407, I found the existing wording confusing as I
expected that the guidelines would provide me with one recommended way
to author function arguments in all situations. However, the
discussion on the issue clarified that the intent was to allow many
ways to author functions, and only to disallow specific clearly
non-optimal cases.

This is my attempt to reduce that confusion, by explicitly calling out
when such freedoms exist.

As I mentioned also in #1407, I'm somewhat doubtful that providing
this much developer freedom is a good idea. A different way to solve this confusion
would have been to provide an explicit recommendation for these ambiguous cases.
However, I don't feel especially qualified to provide such a recommendation, hence
this PR.

As I discussed in isocpp#1407, I found the existing wording confusing as I
expected that the guidelines would provide me with one recommended way
to author function arguments in all situations.  However, the
discussion on the issue clarified that the intent was to allow many
ways to author functions, and only to disallow specific clearly
non-optimal cases.

This is my attempt to reduce that confusion, by explicitly calling out
when such freedoms exist.
@hsutter
Copy link
Contributor

hsutter commented May 2, 2019

Editors call: Thanks, we've reviewed this and we think we see where you're coming from.

We believe that the guidance does actually provide a deterministic decision tree for how to write parameters. Note that each Item is about "what I want to do with the argument" -- e.g., "in" means "I want an X I can read from." So for example the last edit for F.18, which correctly notes that const& and && overloads can be used, doesn't actually belong in that item because F.18 is intended to be about "my functions wants an argument I will move from," and in that case there will not be a const& overload. Rather, the const&/&& overloading comes from the "in" parameter (F.16), where we use by-value or by-const& by default, then only if we want to optimize we can add a && overload. So we think that the guidance as written is correct, based on each Item being about wanting to do a specific thing with the argument.

With that in mind, are there other changes that would make that clearer?

@russellmcc
Copy link
Contributor Author

russellmcc commented May 2, 2019

Thanks for your time. I really appreciate the consideration and thought, as well as the rest of the work the editors do on this document.

I completely see your point regarding my note in F.18 being inappropriate. I'll drop that part without further comment.

However, unfortunately, I just can't agree with you that the guidelines as written provide a deterministic method for choosing a function's signature. As I mentioned in the discussion on the linked issue (#1407), many rules can apply to a given scenario, and the exact set of signature(s) you end up with depends deeply on which rule you choose to apply.

struct A { vector<int> data; };
struct B { vector<int> data; };


// Follows F.16 - no optimization
B convert(const A& a) {
 return B{a.data};
}

// Follows F.16 - with optimization
B convert(const A& a) {
 return B{a.data};
}

B convert(A&& a) {
  return B{std::move(a).data};
}

// Follows F.18
B convert(A&& a) {
  return B{std::move(a).data};
}

// Follows F.19
template <typename A>
B convert(A&& a) {
 return B{std::forward<A>(a).data};
}

So, from the same starting point ("I want to convert A to B"), the guidelines can point me in four different directions regarding the type signatures in the overload set. To me, seeing the chart after F.15 made me think there would be a single recommended overload set for each scenario. This is what my edit was trying to clarify.

I'm totally open to the idea that I'm missing something about how I'm supposed to think about this set of guidelines - if I am, please let me know!

@hsutter hsutter self-assigned this May 9, 2019
@@ -2847,9 +2849,9 @@ When copying is cheap, nothing beats the simplicity and safety of copying, and f

For advanced uses (only), where you really need to optimize for rvalues passed to "input-only" parameters:

* If the function is going to unconditionally move from the argument, take it by `&&`. See [F.18](#Rf-consume).
* If the function is going to unconditionally move from the argument, take it by `&&`. See [F.18](#Rf-consume). You may also provide a `const &` overload that copies the argument rather than moves it, although this is not required.
Copy link

@palotasb palotasb May 10, 2019

Choose a reason for hiding this comment

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

Since this is under the text For [...] where you really need to optimize for rvalues [...] I think this change is a little bit redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This came from a specific point I was confused by, which is that the guidelines allow you to provide only an && overload in these situations. To me, this is a useful clarification since it explicitly says that you're not required to provide a const & overload as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the shared_ptr guidance in R.34 (and the discussion in #1989), should F.18 be updated to allow pass by value and then move?

// Follows F.18
B convert(A&& a) {
  return B{std::move(a).data};
}

or

// Also follows F.18
B convert(A a) {
  return B{std::move(a).data};
}

Pass by value is simpler than const& and && overloads but does result in an extra move. I sometimes pass vectors this way in my own code. F.18 already has an exception for unique_ptr, should another exception be added for cheap to move types?

Copy link

Choose a reason for hiding this comment

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

Those are two different cases: A&& forces a r- value to be passed (this is the classic "consume" semantic. A is a simplified version of the const A& and A&& overload set, which means I will keep a copy.

Copy link

Choose a reason for hiding this comment

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

I cant move from a const&, so this change seems wrong to me.

Copy link
Contributor Author

@russellmcc russellmcc Nov 6, 2022

Choose a reason for hiding this comment

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

What this change meant to convey is - If you wrote a move-only sink function, it must use &&. However, it may be more convenient for the caller if you provide both a && and const & overload, and this is indeed also allowed by the guidelines. As written I've seen many people be confused by this note, since they thought since they wrote a function that always moves an argument, they are somehow not allowed to provide a const & overload. They probably got this impression because they only read this note and did not perform a close reading of the full set of guidelines F.16-F.18. That's the confusion I'm trying to avoid by editing this note. Could you suggest a better way to make this clarification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding @bgloyer 's suggestion to change the guidelines to allow pass-by-value, I'm in support of this but this would be more disruptive than the change I was trying to make on this PR. In particular, I'm a big fan of the alternative function passing guidelines proposed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the first question is should the F.call guidelines be updated to allow pass by value and move if the callee consumes the parameter? Then if so, where should it go?

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.

6 participants