Skip to content

Commit aea85b1

Browse files
committed
Modify R.34 and delete R.36 to conform to F.16-F.18
As it stands now, to my understanding R.34 conflicts with F.16-F.18. R.34 suggests taking `shared_ptr` by value in a "sink" function, but F.18 mentions explicitly that these sort of functions should pass by `&&`, and specifically _not_ by value. This is mentioned by @hsutter in [this comment](#1916 (comment)) So, this PR attempts to resolve the conflict by preferring the "F.16-F.18" guidelines and attempting to modify the R guidelines to fit. There are a number of other ways to avoid this ambiguity, for example we could explicitly state that `shared_ptr` is not subject to F.16-F.18 somewhere in those guidelines. R.34 could also be made to fit if it were explicitly stated that `shared_ptr` be considered "cheap to copy", although I don't see how R.36 could be made to fit with F.16-F.18. I don't have any strong opinion on what the "right" resolution is here, but the fact that these two sets of guidelines seem to disagree has been a source of confusion.
1 parent bf29c35 commit aea85b1

File tree

1 file changed

+22
-30
lines changed

1 file changed

+22
-30
lines changed

CppCoreGuidelines.md

Lines changed: 22 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -2764,7 +2764,7 @@ Passing a shared smart pointer (e.g., `std::shared_ptr`) implies a run-time cost
27642764
void g(unique_ptr<int>);
27652765

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

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

93059304
### <a name="Rr-raii"></a>R.1: Manage resources automatically using resource handles and RAII (Resource Acquisition Is Initialization)
@@ -9994,7 +9993,7 @@ Using `unique_ptr` in this way both documents and enforces the function call's r
99949993
* (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.
99959994
* (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.
99969995

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

99999998
##### Reason
100009999

@@ -10005,19 +10004,33 @@ This makes the function's ownership sharing explicit.
1000510004
class WidgetUser
1000610005
{
1000710006
public:
10008-
// WidgetUser will share ownership of the widget
10009-
explicit WidgetUser(std::shared_ptr<widget> w) noexcept:
10007+
// WidgetUser will share ownership of the widget, this is a "sink" function
10008+
// following F.18
10009+
explicit WidgetUser(std::shared_ptr<widget>&& w) noexcept:
1001010010
m_widget{std::move(w)} {}
1001110011
// ...
1001210012
private:
1001310013
std::shared_ptr<widget> m_widget;
1001410014
};
1001510015

10016+
##### Example, good
10017+
10018+
class WidgetUser
10019+
{
10020+
public:
10021+
// WidgetUser will share ownership of the widget, this is an "in" parameter
10022+
// following F.16
10023+
explicit WidgetUser(const std::shared_ptr<widget>& w) noexcept:
10024+
m_widget{w} {}
10025+
// ...
10026+
private:
10027+
std::shared_ptr<widget> m_widget;
10028+
};
10029+
1001610030
##### Enforcement
1001710031

1001810032
* (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.
10019-
* (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.
10020-
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by rvalue reference. Suggesting taking it by value instead.
10033+
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by revalue 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.
1002110034

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

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

1004210055
* (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.
10043-
* (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.
10044-
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by rvalue reference. Suggesting taking it by value instead.
10045-
10046-
### <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 ???
10047-
10048-
##### Reason
10049-
10050-
This makes the function's ??? explicit.
10051-
10052-
##### Example, good
10053-
10054-
void share(shared_ptr<widget>); // share -- "will" retain refcount
10055-
10056-
void reseat(shared_ptr<widget>&); // "might" reseat ptr
10057-
10058-
void may_share(const shared_ptr<widget>&); // "might" retain refcount
10059-
10060-
##### Enforcement
10061-
10062-
* (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.
10063-
* (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.
10064-
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by rvalue reference. Suggesting taking it by value instead.
10056+
* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer<T>` by revalue 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.
1006510057

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

0 commit comments

Comments
 (0)