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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0edfca6
Explain freedoms available in F.16-F.18
russellmcc Apr 28, 2019
7802043
Remove note in F.18
russellmcc May 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions CppCoreGuidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2825,6 +2825,8 @@ Advanced parameter passing:
![Advanced parameter passing table](./param-passing-advanced.png "Advanced parameter passing")

Use the advanced techniques only after demonstrating need, and document that need in a comment.
Note that the guidelines do not provide a complete decision procedure on how to write a function.
In many situations, there will be multiple ways to write a function that is in accordance with the guidelines.

### <a name="Rf-in"></a>F.16: For "in" parameters, pass cheaply-copied types by value and others by reference to `const`

Expand All @@ -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 •
edited
Loading

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 •
edited
Loading

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?

* If the function is going to keep a copy of the argument, in addition to passing by `const&` (for lvalues),
add an overload that passes the parameter by `&&` (for rvalues) and in the body `std::move`s it to its destination. Essentially this overloads a "will-move-from"; see [F.18](#Rf-consume).
add an overload that passes the parameter by `&&` (for rvalues) and in the body `std::move`s it to its destination. Essentially this overloads a "will-move-from"; see [F.18](#Rf-consume). It also could be appropriate to remove the `const&` overload and only keep the `&&` overload.
* In special cases, such as multiple "input + copy" parameters, consider using perfect forwarding. See [F.19](#Rf-forward).

##### Example
Expand Down