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

I.11: "Never transfer ownership by a raw pointer (T*)" doesn't mention transfer by method parameter #522

Closed
murraycu opened this Issue Feb 8, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@murraycu
Copy link

murraycu commented Feb 8, 2016

I.11: Never transfer ownership by a raw pointer (T*)
discusses how to transfer ownership as a return type. But shouldn't it also discuss transferring ownership via a method parameter? I guess it would look like this, from the caller:

auto something = std::make_unique("something");
...
foo.set_something(std::move(something));

These other sections don't mention this either:
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-unique_ptr
(discusses return types only)
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#f7-for-general-use-take-t-or-t-arguments-rather-than-smart-pointers
(only has an example of when not to take a smart pointer (shared_ptr) as a parameter).

@villintehaspam

This comment has been minimized.

Copy link

villintehaspam commented Feb 9, 2016

The rule does have a link to F.15: Prefer simple and conventional ways of passing information on the topic of argument passing.

So I would argue that the information is present in F.15 + F.18, and that the link in I.11 is enough.

@murraycu

This comment has been minimized.

Copy link

murraycu commented Feb 9, 2016

Thanks. If the advice is then not to take ownership like so:
void set_something(std::unique_ptr something)
then I think it could be much clearer about it. But I don't think that's what the document is trying to say.

F.15 seems to be more a discussion of whether the type should be passed by ref, const ref, ref ref, etc, rather than what the underlying type should be.

F.18 (which has no example) does mention "Unique owner types that are move-only and cheap-to-move, such as unique_ptr, can also be passed by value" but I'd far rather have an explicit obvious example in I.11 which actually discusses why and when you'd use owner and unique_ptr (as parameters), rather than exactly how to pass them.

@murraycu

This comment has been minimized.

Copy link

murraycu commented Feb 9, 2016

By the way, one reason that I'd like to see this spelled out clearly, is that it would sometimes require use of std::move() by the caller, correctly I think. But that's something that's sometimes discouraged and there's even a rule against it:
ES.56: Avoid std::move() in application code

However, see #514

@murraycu

This comment has been minimized.

Copy link

murraycu commented Feb 9, 2016

Actually, "R.32: Take a unique_ptr parameter to express that a function assumes ownership of a widget" does mention exactly this, so maybe it should just be linked to from I.11.

I feel that the advice about this is spread rather widely across the whole document.

@murraycu

This comment has been minimized.

Copy link

murraycu commented Feb 26, 2016

By the way, one reason that I'd like to see this spelled out clearly, is that it would sometimes
require use of std::move() by the caller, correctly I think. But that's something that's sometimes
discouraged and there's even a rule against it:
ES.56: Avoid std::move() in application code
However, see #514

That section has now been rewritten, with some examples of when you should use std::move when passing parameters. I guess I.11 should have a link to that too, as well as a link to R.32. Though really, it seems that all these sections should maybe just be combined or linked to some combined example.

@AndrewPardoe

This comment has been minimized.

Copy link
Contributor

AndrewPardoe commented May 15, 2017

Per our editors' discussion, these links are useful. Thank you for the suggestion!

@BjarneStroustrup BjarneStroustrup added resolved and removed open labels May 17, 2017

@BjarneStroustrup

This comment has been minimized.

Copy link
Contributor

BjarneStroustrup commented May 17, 2017

added xrefs as suggested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment