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

Why Resource-Management is no discussion about shared_ptr<widget>&& and unique_ptr<widget>&& ? #1916

Open
ltimaginea opened this issue May 25, 2022 · 2 comments
Assignees

Comments

@ltimaginea
Copy link

ltimaginea commented May 25, 2022

Should add the discussion about shared_ptr<widget>&& and unique_ptr<widget>&& in Smart pointer rule summary .

Link: Smart pointer rule summary

For R.34's example code, when enter the function, the shared_ptr is copy-constructed, and this requires incrementing the strong reference count. This can incur performance costs. But if the parameter type is shared_ptr<widget>&& , this directly addresses the cost. The rvalue reference type shared_ptr<widget>&& is more efficient than the value type shared_ptr<widget> . So, I don't understand R.34 Enforcement's final line:

  • (Simple) ((Foundation)) Warn if a function takes a Shared_pointer<T> by rvalue reference. Suggesting taking it by value instead.

I modified the example code for R.34 , I think the example code should be written like this:

class WidgetUser
{
public:
    // the parameter type is shared_ptr<widget>&&
    explicit WidgetUser(std::shared_ptr<widget>&& w) noexcept:
        m_widget{std::move(w)} {}
    // ...
private:
    std::shared_ptr<widget> m_widget;
};

On the other hand, R.32 and R.33 talk about unique_ptr<widget> and unique_ptr<widget>& . But I think, unique_ptr<widget>&& is also very useful. If we take a unique_ptr<widget> parameter, its total cost would be two moves. But if we take a unique_ptr<widget>&& parameter, the total cost is one move.

The screenshots from the Scott Meyers's Effective Modern C++ Item41 is as follows:
Effective Modern C++ Item41

So, I think we should add the discussion about shared_ptr<widget>&& and unique_ptr<widget>&& in Smart pointer rule summary .

@bgloyer
Copy link
Contributor

bgloyer commented Jun 2, 2022

This was mentioned in Issue #1781 for shared_ptr

@hsutter
Copy link
Contributor

hsutter commented Jul 13, 2022

Editors call: F.18 covers that for "sink" functions (for smart pointers, ownership transfer) to pass by &&. In our desire to keep the smart pointer passing rules simple and avoid teaching &&, we covered only unique_ptr "sink" functions where pass by value happens to have the same effect because unique_ptr happens to be move-only, but we did not cover other smart pointer "sink" functions. Those should pass by &&. We might add a guideline saying so, or perhaps add a note saying so.

russellmcc added a commit to russellmcc/CppCoreGuidelines that referenced this issue Oct 26, 2022
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](isocpp#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.
russellmcc added a commit to russellmcc/CppCoreGuidelines that referenced this issue Oct 26, 2022
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](isocpp#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.
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

No branches or pull requests

4 participants