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

Modify R.34 and delete R.36 to make them conform to F.16-F.18 #1989

Closed
wants to merge 1 commit into from
Closed

Modify R.34 and delete R.36 to make them conform to F.16-F.18 #1989

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 22 additions & 30 deletions CppCoreGuidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2764,7 +2764,7 @@ Passing a shared smart pointer (e.g., `std::shared_ptr`) implies a run-time cost
void g(unique_ptr<int>);

// can only accept ints for which you are willing to share ownership
void g(shared_ptr<int>);
void g(const shared_ptr<int>&);

// doesn't change ownership, but requires a particular ownership of the caller
void h(const unique_ptr<int>&);
Expand Down Expand Up @@ -9297,9 +9297,8 @@ Here, we ignore such cases.
* [R.31: If you have non-`std` smart pointers, follow the basic pattern from `std`](#Rr-smart)
* [R.32: Take a `unique_ptr<widget>` parameter to express that a function assumes ownership of a `widget`](#Rr-uniqueptrparam)
* [R.33: Take a `unique_ptr<widget>&` parameter to express that a function reseats the `widget`](#Rr-reseat)
* [R.34: Take a `shared_ptr<widget>` parameter to express shared ownership](#Rr-sharedptrparam-owner)
* [R.34: Take a `shared_ptr<widget>&&` or `const shared_ptr<widget>&` parameter to express shared ownership](#Rr-sharedptrparam-owner)
* [R.35: Take a `shared_ptr<widget>&` parameter to express that a function might reseat the shared pointer](#Rr-sharedptrparam)
* [R.36: Take a `const shared_ptr<widget>&` parameter to express that it might retain a reference count to the object ???](#Rr-sharedptrparam-const)
* [R.37: Do not pass a pointer or reference obtained from an aliased smart pointer](#Rr-smartptrget)

### <a name="Rr-raii"></a>R.1: Manage resources automatically using resource handles and RAII (Resource Acquisition Is Initialization)
Expand Down Expand Up @@ -9994,7 +9993,7 @@ Using `unique_ptr` in this way both documents and enforces the function call's r
* (Simple) Warn if a function takes a `Unique_pointer<T>` parameter by lvalue reference and does not either assign to it or call `reset()` on it on at least one code path. Suggest taking a `T*` or `T&` instead.
* (Simple) ((Foundation)) Warn if a function takes a `Unique_pointer<T>` parameter by reference to `const`. Suggest taking a `const T*` or `const T&` instead.

### <a name="Rr-sharedptrparam-owner"></a>R.34: Take a `shared_ptr<widget>` parameter to express shared ownership
### <a name="Rr-sharedptrparam-owner"></a>R.34: Take a `shared_ptr<widget>&&` or `const shared_ptr<widget>&` parameter to express shared ownership
Copy link
Contributor

Choose a reason for hiding this comment

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

Continue to allow pass by value? It is arguable that a shared_ptr is "cheap to copy". Also rule F.15 says to keep parameter passing simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Could we get an official ruling on whether shared_ptr is "cheap to copy"? This PR is assuming it was not cheap to copy, since copying incurs an "acquire-release" synchronization in the destructor. I don't have a strong opinion here but I believe it is ambiguous enough that we should have an official call about this in the guidelines.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for cheap to copy. In practice, the performance of passing a shared pointer wont matter because typically the time spent using the contents of the shared pointer will be much larger than the time spent passing it. Also the ref count change can be avoided if the caller moves it's shared pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no strong opinion. I can see your argument but there are functions that are so cheap that the copy overhead could matter. The current guidance for cheap to copy types tends to imply only types where the overhead is so low as to “never matter” should count. Would it help matters if I wrote an alternative PR from the “shared_ptr is cheap to copy” point of view? My main goal here is to have a consistent set of guidelines.


##### Reason

Expand All @@ -10005,19 +10004,33 @@ This makes the function's ownership sharing explicit.
class WidgetUser
{
public:
// WidgetUser will share ownership of the widget
explicit WidgetUser(std::shared_ptr<widget> w) noexcept:
// WidgetUser will share ownership of the widget, this is a "sink" function
// following F.18
explicit WidgetUser(std::shared_ptr<widget>&& w) noexcept:
Copy link
Contributor

Choose a reason for hiding this comment

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

Also have pass by const ref here? Interfaces that only accept rvalue ref's can be inconvient to use. It is possible that someone wanting to call a constructor like this would have an lvalue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

F.18 mentions that sink functions like this one should take arguments by rvalue reference. I'm not sure whether users are required to provide another overload with const ref, I believe it is allowed but not required. I tried to clarify this on my other PR but that has not been landed yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

A sink function is requiring the caller to get an rvalue which can put a burden on the caller. The caller may want to keep its own copy of the shared pointer while 'sharing' it with the callee.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t mind adding an overload but if this is a general requirement we should also modify F.18 to make that clear. As written F.18 doesn’t seem like it requires authors to add a const ref overload.

Copy link

@MikeGitb MikeGitb Nov 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That discussion is pretty much the motivation for recommending pass-by-value by default for shared_ptr. If the caller has an r- value you have a move, if not you get a copy anyway. I know, it is less optimal than having two overloads, but if you knew it makes a difference in your context, you probably also know what to do best this context and otherwise your saving yourself the hassle of implementing the function twice.

That aside: An API that consumes a shared pointer (i.e. forces the caller to pass a r-value shared_ptr) should be rather rare. Consume usually means "I'm going to take the object and do with it as I please - you (the callet) shouldn't access it after I have consumed it". A shared_ptr on the other hand expresses the exact opposite semantics, so 9 out of 10, if you have a shared_ptr<T>&& parameter it will be an optimization - not the primary interface. The only exception I can think of is when you know this function will only be used in a pass-through callchain (i.e. a helper/implementation function)..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you suggest we resolve the incompatibility of r.34 with f.16-f.18? I understand your points and I agree but I’m struggling to synthesize them into concrete changes to this pr.

Copy link
Contributor

Choose a reason for hiding this comment

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

True. F.16-18 (F.18 specifically) do not seem to cover this kind of parameter passing.

m_widget{std::move(w)} {}
// ...
private:
std::shared_ptr<widget> m_widget;
};

##### Example, good

class WidgetUser
{
public:
// WidgetUser will share ownership of the widget, this is an "in" parameter
// following F.16
explicit WidgetUser(const std::shared_ptr<widget>& w) noexcept:
m_widget{w} {}
// ...
private:
std::shared_ptr<widget> m_widget;
};

##### Enforcement

* (Simple) Warn if a function takes a `Shared_pointer<T>` parameter by lvalue reference and does not either assign to it or call `reset()` on it on at least one code path. Suggest taking a `T*` or `T&` instead.
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by value or by reference to `const` and does not copy or move it to another `Shared_pointer` on at least one code path. Suggest taking a `T*` or `T&` instead.
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by rvalue reference. Suggesting taking it by value instead.
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by rvalue reference or by reference to `const` and does not copy or move it to another `Shared_pointer` on at least one code path. Suggest taking a `T*` or `T&` instead.

### <a name="Rr-sharedptrparam"></a>R.35: Take a `shared_ptr<widget>&` parameter to express that a function might reseat the shared pointer

Expand All @@ -10040,28 +10053,7 @@ This makes the function's reseating explicit.
##### Enforcement

* (Simple) Warn if a function takes a `Shared_pointer<T>` parameter by lvalue reference and does not either assign to it or call `reset()` on it on at least one code path. Suggest taking a `T*` or `T&` instead.
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by value or by reference to `const` and does not copy or move it to another `Shared_pointer` on at least one code path. Suggest taking a `T*` or `T&` instead.
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by rvalue reference. Suggesting taking it by value instead.

### <a name="Rr-sharedptrparam-const"></a>R.36: Take a `const shared_ptr<widget>&` parameter to express that it might retain a reference count to the object ???

##### Reason

This makes the function's ??? explicit.

##### Example, good

void share(shared_ptr<widget>); // share -- "will" retain refcount

void reseat(shared_ptr<widget>&); // "might" reseat ptr

void may_share(const shared_ptr<widget>&); // "might" retain refcount

##### Enforcement

* (Simple) Warn if a function takes a `Shared_pointer<T>` parameter by lvalue reference and does not either assign to it or call `reset()` on it on at least one code path. Suggest taking a `T*` or `T&` instead.
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by value or by reference to `const` and does not copy or move it to another `Shared_pointer` on at least one code path. Suggest taking a `T*` or `T&` instead.
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by rvalue reference. Suggesting taking it by value instead.
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by rvalue reference or by reference to `const` and does not copy or move it to another `Shared_pointer` on at least one code path. Suggest taking a `T*` or `T&` instead.

### <a name="Rr-smartptrget"></a>R.37: Do not pass a pointer or reference obtained from an aliased smart pointer

Expand Down