Modify R.34 and delete R.36 to make them conform to F.16-F.18 #1989
Conversation
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
force-pushed
the
modify-r-34-to-conform-with-f-18
branch
from
aea85b1
to
a21195d
Compare
| @@ -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<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. | |||
| * (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. | |||
|
|
|||
| ### <a name="Rr-sharedptrparam-owner"></a>R.34: Take a `shared_ptr<widget>` parameter to express shared ownership | |||
| ### <a name="Rr-sharedptrparam-owner"></a>R.34: Take a `shared_ptr<widget>&&` or `const shared_ptr<widget>&` parameter to express shared ownership | |||
There was a problem hiding this comment.
Continue to allow pass by value? It is arguable that a shared_ptr is "cheap to copy". Also rule F.15 says to keep parameter passing simple.
There was a problem hiding this comment.
Thanks. Could we get an official ruling on whether shared_ptr is "cheap to copy"? This PR is assuming it was not cheap to copy, since copying incurs an "acquire-release" synchronization in the destructor. I don't have a strong opinion here but I believe it is ambiguous enough that we should have an official call about this in the guidelines.
There was a problem hiding this comment.
I'd vote for cheap to copy. In practice, the performance of passing a shared pointer wont matter because typically the time spent using the contents of the shared pointer will be much larger than the time spent passing it. Also the ref count change can be avoided if the caller moves it's shared pointer.
There was a problem hiding this comment.
I have no strong opinion. I can see your argument but there are functions that are so cheap that the copy overhead could matter. The current guidance for cheap to copy types tends to imply only types where the overhead is so low as to “never matter” should count. Would it help matters if I wrote an alternative PR from the “shared_ptr is cheap to copy” point of view? My main goal here is to have a consistent set of guidelines.
| explicit WidgetUser(std::shared_ptr<widget> w) noexcept: | ||
| // WidgetUser will share ownership of the widget, this is a "sink" function | ||
| // following F.18 | ||
| explicit WidgetUser(std::shared_ptr<widget>&& w) noexcept: |
There was a problem hiding this comment.
Also have pass by const ref here? Interfaces that only accept rvalue ref's can be inconvient to use. It is possible that someone wanting to call a constructor like this would have an lvalue.
There was a problem hiding this comment.
F.18 mentions that sink functions like this one should take arguments by rvalue reference. I'm not sure whether users are required to provide another overload with const ref, I believe it is allowed but not required. I tried to clarify this on my other PR but that has not been landed yet.
There was a problem hiding this comment.
A sink function is requiring the caller to get an rvalue which can put a burden on the caller. The caller may want to keep its own copy of the shared pointer while 'sharing' it with the callee.
There was a problem hiding this comment.
I don’t mind adding an overload but if this is a general requirement we should also modify F.18 to make that clear. As written F.18 doesn’t seem like it requires authors to add a const ref overload.
There was a problem hiding this comment.
That discussion is pretty much the motivation for recommending pass-by-value by default for shared_ptr. If the caller has an r- value you have a move, if not you get a copy anyway. I know, it is less optimal than having two overloads, but if you knew it makes a difference in your context, you probably also know what to do best this context and otherwise your saving yourself the hassle of implementing the function twice.
That aside: An API that consumes a shared pointer (i.e. forces the caller to pass a r-value shared_ptr) should be rather rare. Consume usually means "I'm going to take the object and do with it as I please - you (the callet) shouldn't access it after I have consumed it". A shared_ptr on the other hand expresses the exact opposite semantics, so 9 out of 10, if you have a shared_ptr<T>&& parameter it will be an optimization - not the primary interface. The only exception I can think of is when you know this function will only be used in a pass-through callchain (i.e. a helper/implementation function)..
There was a problem hiding this comment.
How do you suggest we resolve the incompatibility of r.34 with f.16-f.18? I understand your points and I agree but I’m struggling to synthesize them into concrete changes to this pr.
There was a problem hiding this comment.
True. F.16-18 (F.18 specifically) do not seem to cover this kind of parameter passing.
|
Any thoughts on how we might resolve this inconsistency in the guidelines? Not having a clear recommendation for passing |
I think those guidelines could link to the R guidelines (r.34 in particular). And name them as an exception. F 16-F18 describe the general case, R.34 a very specific case, where other considerations take precedence. |
Where does R.34 talk about sink functions? |
|
I'll open a new PR preferring the "R" rules for |
Currently these guidelines conflict with R.34, R.35, and R.36. This conflict has led to confusion, where it's unclear which guidelines to prefer for `shared_ptr` types. In a [previous PR](isocpp#1989) I proposed preferring the "F" series of guidelines. This PR takes the opposite approach and prefers the "R" guidelines for `shared_ptr` types. I don't feel strongly about which guidelines to prefer, I just want to make sure the guidelines are internally consistent.
|
opened #2010 |
Currently these guidelines conflict with R.34, R.35, and R.36. This conflict has led to confusion, where it's unclear which guidelines to prefer for `shared_ptr` types. In a [previous PR](isocpp#1989) I proposed preferring the "F" series of guidelines. This PR takes the opposite approach and prefers the "R" guidelines for `shared_ptr` types. I don't feel strongly about which guidelines to prefer, I just want to make sure the guidelines are internally consistent.
Currently these guidelines conflict with R.34, R.35, and R.36. This conflict has led to confusion, where it's unclear which guidelines to prefer for `shared_ptr` types. In a [previous PR](isocpp#1989) I proposed preferring the "F" series of guidelines. This PR takes the opposite approach and prefers the "R" guidelines for `shared_ptr` types. I don't feel strongly about which guidelines to prefer, I just want to make sure the guidelines are internally consistent.
Currently these guidelines conflict with R.34, R.35, and R.36. This conflict has led to confusion, where it's unclear which guidelines to prefer for `shared_ptr` types. In a [previous PR](#1989) I proposed preferring the "F" series of guidelines. This PR takes the opposite approach and prefers the "R" guidelines for `shared_ptr` types. I don't feel strongly about which guidelines to prefer, I just want to make sure the guidelines are internally consistent.
As it stands now, to my understanding R.34 and R.36 conflict with F.16-F.18.
R.34 suggests taking
shared_ptrby 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 commentSo, 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 conflict, for example we could explicitly state that
shared_ptris not subject to F.16-F.18 somewhere in those guidelines. R.34 could also be made to fit if it were explicitly stated thatshared_ptrbe 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.