From a21195d9f44cb817906e5515202718b211891de1 Mon Sep 17 00:00:00 2001 From: Russell McClellan Date: Wed, 26 Oct 2022 15:23:10 -0400 Subject: [PATCH] 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](https://github.com/isocpp/CppCoreGuidelines/issues/1916#issuecomment-1183468209) 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. --- CppCoreGuidelines.md | 52 +++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 9af57c962..d8f27842e 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -2764,7 +2764,7 @@ Passing a shared smart pointer (e.g., `std::shared_ptr`) implies a run-time cost void g(unique_ptr); // can only accept ints for which you are willing to share ownership - void g(shared_ptr); + void g(const shared_ptr&); // doesn't change ownership, but requires a particular ownership of the caller void h(const unique_ptr&); @@ -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` parameter to express that a function assumes ownership of a `widget`](#Rr-uniqueptrparam) * [R.33: Take a `unique_ptr&` parameter to express that a function reseats the `widget`](#Rr-reseat) - * [R.34: Take a `shared_ptr` parameter to express shared ownership](#Rr-sharedptrparam-owner) + * [R.34: Take a `shared_ptr&&` or `const shared_ptr&` parameter to express shared ownership](#Rr-sharedptrparam-owner) * [R.35: Take a `shared_ptr&` parameter to express that a function might reseat the shared pointer](#Rr-sharedptrparam) - * [R.36: Take a `const shared_ptr&` 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) ### 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 * (Simple) Warn if a function takes a `Unique_pointer` 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` parameter by reference to `const`. Suggest taking a `const T*` or `const T&` instead. -### R.34: Take a `shared_ptr` parameter to express shared ownership +### R.34: Take a `shared_ptr&&` or `const shared_ptr&` parameter to express shared ownership ##### Reason @@ -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 w) noexcept: + // WidgetUser will share ownership of the widget, this is a "sink" function + // following F.18 + explicit WidgetUser(std::shared_ptr&& w) noexcept: m_widget{std::move(w)} {} // ... private: std::shared_ptr 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& w) noexcept: + m_widget{w} {} + // ... + private: + std::shared_ptr m_widget; + }; + ##### Enforcement * (Simple) Warn if a function takes a `Shared_pointer` 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` 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` by rvalue reference. Suggesting taking it by value instead. +* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer` 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. ### R.35: Take a `shared_ptr&` parameter to express that a function might reseat the shared pointer @@ -10040,28 +10053,7 @@ This makes the function's reseating explicit. ##### Enforcement * (Simple) Warn if a function takes a `Shared_pointer` 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` 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` by rvalue reference. Suggesting taking it by value instead. - -### R.36: Take a `const shared_ptr&` 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); // share -- "will" retain refcount - - void reseat(shared_ptr&); // "might" reseat ptr - - void may_share(const shared_ptr&); // "might" retain refcount - -##### Enforcement - -* (Simple) Warn if a function takes a `Shared_pointer` 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` 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` by rvalue reference. Suggesting taking it by value instead. +* (Simple) ((Foundation)) Warn if a function takes a `Shared_pointer` 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. ### R.37: Do not pass a pointer or reference obtained from an aliased smart pointer