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

Allow multiple calls with shared pointers arguments (not being a reference parameter) for rule R.21 #2226

Closed
jansen-tiobe opened this issue Oct 14, 2024 · 3 comments

Comments

@jansen-tiobe
Copy link

I would like to thank Danny Havenith for bringing up this example and suggesting this improvement.

Rule R.21 ("Prefer unique_ptr over shared_ptr unless you need to share ownership") has enforcement:

(Simple) Warn if a function uses a Shared_pointer with an object allocated within the function, but never returns the Shared_pointer or passes it to a function requiring a Shared_pointer&. Suggest using unique_ptr instead.

Now consider the following code:

void Registry::add( shared_pointer< Participant> w);

void f( Registry &r1, Registry &r2)
{
    const auto p = make_shared<Participant>();
    r1.add( p);
    r2.add( p);
}

According to the enforcement this code will be flagged, but it is not possible to use a unique_ptr in this case because the pointer is shared between 2 function calls.

Hence, I suggest to extend the enforcement as follows:

_(Simple) Warn if a function uses a Shared_pointer with an object allocated within the function, but:

  • never returns the Shared_pointer and
  • never passes it to a function requiring a Shared_pointer& and
  • does not pass it to a function as a Shared_pointer argument (by value or reference) more than once.

Suggest using unique_ptr instead._

What are your thoughts? Should I just create a pull request for this change?

@jwakely
Copy link
Contributor

jwakely commented Oct 14, 2024

I don't know why the rule is written in terms of Shared_pointer& as a reference argument (and it should be shared_ptr).

@jwakely
Copy link
Contributor

jwakely commented Oct 14, 2024

Ah, because if the function takes shared_ptr<Participant> by value then you could pass a unique_ptr as an rvalue, as foo(std::move(p))

But I think your proposed change is still too restrictive. Any use of the shared_ptr after the first call to add requires that it hasn't been moved from. This couldn't be a unique_ptr:

void f( Registry& r1)
{
    const auto p = make_shared<Participant>();
    r1.add(p);
    foo(*p);
}

This would still get a warning with your suggested rewrite.

@hsutter
Copy link
Contributor

hsutter commented Oct 24, 2024

Editors call: We agree the & should be removed. Thanks!

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

3 participants