Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2466103002:
Styleguide cleanup part 3. (Closed)
|
Created:
4 years, 1 month ago by Peter Kasting Modified:
4 years, 1 month ago Reviewers:
danakj CC:
chromium-reviews, danakj+watch_chromium.org, jbroman+cpp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStyleguide cleanup part 3.
* Add missing discussion thread links
* Canonicalize existing links
* Standardize pattern for contents of final column: notes, then Google Style Guide link, then discussion thread links
* Move std::nullptr_t from a note in the language features section to its own entry in the library features section
* Remove entry for "containers of move-only types" since AFAICT this isn't really a "feature" with explicit separate documentation, it never had any discussion, and usage of these is commonplace in the codebase now and doesn't need explicit encouragement/clarity
* Updated/clarified some comments
BUG=none
TEST=none
Committed: https://crrev.com/0cb5bd450218c4422cf3b7cb77032d1643a872e8
Cr-Commit-Position: refs/heads/master@{#429399}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Review comments #Patch Set 3 : Reword #Messages
Total messages: 18 (7 generated)
Description was changed from ========== Styleguide cleanup part 3. * Add missing discussion thread links * Canonicalize existing links * Standardize pattern for contents of final column: notes, then Google Style Gudie link, then discussion thread links * Move std::nullptr_t from a note in the language features section to its own entry in the library features section * Remove entry for "containers of move-only types" since AFAICT this isn't really a "feature" with explicit separate documentation, it never had any discussion, and usage of these is commonplace in the codebase now and doesn't need explicit encouragement/clarity * Remove some outdated comments (e.g. talking about scoped_ptr, which no longer exists) BUG=none TEST=none ========== to ========== Styleguide cleanup part 3. * Add missing discussion thread links * Canonicalize existing links * Standardize pattern for contents of final column: notes, then Google Style Guide link, then discussion thread links * Move std::nullptr_t from a note in the language features section to its own entry in the library features section * Remove entry for "containers of move-only types" since AFAICT this isn't really a "feature" with explicit separate documentation, it never had any discussion, and usage of these is commonplace in the codebase now and doesn't need explicit encouragement/clarity * Remove some outdated comments (e.g. talking about scoped_ptr, which no longer exists) BUG=none TEST=none ==========
pkasting@chromium.org changed reviewers: + danakj@chromium.org
Description was changed from ========== Styleguide cleanup part 3. * Add missing discussion thread links * Canonicalize existing links * Standardize pattern for contents of final column: notes, then Google Style Guide link, then discussion thread links * Move std::nullptr_t from a note in the language features section to its own entry in the library features section * Remove entry for "containers of move-only types" since AFAICT this isn't really a "feature" with explicit separate documentation, it never had any discussion, and usage of these is commonplace in the codebase now and doesn't need explicit encouragement/clarity * Remove some outdated comments (e.g. talking about scoped_ptr, which no longer exists) BUG=none TEST=none ========== to ========== Styleguide cleanup part 3. * Add missing discussion thread links * Canonicalize existing links * Standardize pattern for contents of final column: notes, then Google Style Guide link, then discussion thread links * Move std::nullptr_t from a note in the language features section to its own entry in the library features section * Remove entry for "containers of move-only types" since AFAICT this isn't really a "feature" with explicit separate documentation, it never had any discussion, and usage of these is commonplace in the codebase now and doesn't need explicit encouragement/clarity * Updated/clarified some comments BUG=none TEST=none ==========
https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#n... styleguide/c++/c++11.html:484: <td>libstdc++ 4.6 has a bug where <code>std::get</code> can return an lvalue-reference when it should return an rvalue-reference. <code>base::get</code> can be used to avoid this. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/0NnCRmMY1xY">Discussion thread</a>. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/VA7Ej2IWS2w">Another discussion thread</a></td> I rewrote this as best I could based on reading the CL in question, which had no associated bug or discussion thread :( AFAICT, we're not advocating people globally use base::get over std::get, just saying it's available for certain scenarios? I tried to make this more clear.
https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (left): https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#o... styleguide/c++/c++11.html:98: <td>Useful in performance-critical situations, with small, fixed-size arrays. In most cases, consider std::vector instead. std::vector is cheaper to std::move and is more widely used. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/pVRQCRWHEU8">Discussion thread</a>.</td> not clear why you're dropping the periods on the last sentences of these blocks https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#o... styleguide/c++/c++11.html:263: <td>Already in common use in the codebase. Approved without discussion.</td> The explanation why it was approved without discussion seems relevant https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#n... styleguide/c++/c++11.html:190: <td>Do not bind or store capturing lambdas outside the lifetime of the stack frame they are defined in. Captureless lambdas can be used with <code>base::Callback</code>, such as <code>base::Bind([](int j){}, i);</code>, because they offer protection against a large class of object lifetime mistakes. Don't use default captures (<code>[=]</code>, <code>[&]</code> – <a href="https://google.github.io/styleguide/cppguide.html#Lambda_expressions">Google Style Guide</a>). Lambdas are typically useful as a parameter to methods or functions that will use them immediately, such as those in <code><algorithm></code>. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/D9UnnxBnciQ">Discussion thread</a>. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/QxjsPELDYdQ">Another discussion thread</a></td> Maybe mention the 2nd thread is about interaction with bind? https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#n... styleguide/c++/c++11.html:484: <td>libstdc++ 4.6 has a bug where <code>std::get</code> can return an lvalue-reference when it should return an rvalue-reference. <code>base::get</code> can be used to avoid this. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/0NnCRmMY1xY">Discussion thread</a>. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/VA7Ej2IWS2w">Another discussion thread</a></td> On 2016/10/31 23:46:48, Peter Kasting wrote: > I rewrote this as best I could based on reading the CL in question, which had no > associated bug or discussion thread :( > > AFAICT, we're not advocating people globally use base::get over std::get, just > saying it's available for certain scenarios? I tried to make this more clear. I'm torn, consistently using base::get is nice but it's a make-work project to convert more code in the future so.. yeh I dont have feelings to direct strongly
https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (left): https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#o... styleguide/c++/c++11.html:98: <td>Useful in performance-critical situations, with small, fixed-size arrays. In most cases, consider std::vector instead. std::vector is cheaper to std::move and is more widely used. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/pVRQCRWHEU8">Discussion thread</a>.</td> On 2016/11/01 00:49:41, danakj wrote: > not clear why you're dropping the periods on the last sentences of these blocks Because it's weird that 98% of the cases say "Discussion thread" and 2% say "Discussion thread.", and those 2% don't agree internally about whether the period is inside or outside the link. I'm borderline OCD :( https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#o... styleguide/c++/c++11.html:263: <td>Already in common use in the codebase. Approved without discussion.</td> On 2016/11/01 00:49:41, danakj wrote: > The explanation why it was approved without discussion seems relevant Hmm, it seemed sort of confusing since you don't know when the vintage of the sentence is (is it "already in use" in 2016? 15? 14? 2006?), and the Google Style Guide mandates these anyway. See if you like my alternative. https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#n... styleguide/c++/c++11.html:190: <td>Do not bind or store capturing lambdas outside the lifetime of the stack frame they are defined in. Captureless lambdas can be used with <code>base::Callback</code>, such as <code>base::Bind([](int j){}, i);</code>, because they offer protection against a large class of object lifetime mistakes. Don't use default captures (<code>[=]</code>, <code>[&]</code> – <a href="https://google.github.io/styleguide/cppguide.html#Lambda_expressions">Google Style Guide</a>). Lambdas are typically useful as a parameter to methods or functions that will use them immediately, such as those in <code><algorithm></code>. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/D9UnnxBnciQ">Discussion thread</a>. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/QxjsPELDYdQ">Another discussion thread</a></td> On 2016/11/01 00:49:41, danakj wrote: > Maybe mention the 2nd thread is about interaction with bind? I rewrote this item. * Put motivation first, then caveats. * Soften language about default captures, since the intent was to capture the spirit of the Google Style Guide, but that changed to allow these since this was written. * Remove phrase about "protection against a large class of lifetime mistakes" since that appeared nowhere in the relevant discussion thread or linked CL and, AFAICT, is not particularly true (the closest thing I found was a comment about how captureless lambdas were not at risk for the same sorts of lifetime mistakes in this case that capturing lambdas are -- but that's not the same thing as implying that when you use a captureless lambda you magically gain lifetime protections). I did also add the explicit mention you ask for, though now that the bit about captureless lambdas and Bind is the last thing in the text and the text is slightly simpler, it feels more redundant than before. Up to you. https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#n... styleguide/c++/c++11.html:484: <td>libstdc++ 4.6 has a bug where <code>std::get</code> can return an lvalue-reference when it should return an rvalue-reference. <code>base::get</code> can be used to avoid this. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/0NnCRmMY1xY">Discussion thread</a>. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/VA7Ej2IWS2w">Another discussion thread</a></td> On 2016/11/01 00:49:41, danakj wrote: > On 2016/10/31 23:46:48, Peter Kasting wrote: > > I rewrote this as best I could based on reading the CL in question, which had > no > > associated bug or discussion thread :( > > > > AFAICT, we're not advocating people globally use base::get over std::get, just > > saying it's available for certain scenarios? I tried to make this more clear. > > I'm torn, consistently using base::get is nice but it's a make-work project to > convert more code in the future so.. yeh I dont have feelings to direct strongly It seems like it's a temporary workaround for libstdc++ 4.6 anyway, right? Once we're on a newer version of that, I'd think we'd want to rip out base::get entirely. So directing people to always use it seems ugh?
LGTM https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#n... styleguide/c++/c++11.html:190: <td>Do not bind or store capturing lambdas outside the lifetime of the stack frame they are defined in. Captureless lambdas can be used with <code>base::Callback</code>, such as <code>base::Bind([](int j){}, i);</code>, because they offer protection against a large class of object lifetime mistakes. Don't use default captures (<code>[=]</code>, <code>[&]</code> – <a href="https://google.github.io/styleguide/cppguide.html#Lambda_expressions">Google Style Guide</a>). Lambdas are typically useful as a parameter to methods or functions that will use them immediately, such as those in <code><algorithm></code>. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/D9UnnxBnciQ">Discussion thread</a>. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/QxjsPELDYdQ">Another discussion thread</a></td> On 2016/11/01 01:34:52, Peter Kasting wrote: > On 2016/11/01 00:49:41, danakj wrote: > > Maybe mention the 2nd thread is about interaction with bind? > > I rewrote this item. > > * Put motivation first, then caveats. > * Soften language about default captures, since the intent was to capture the > spirit of the Google Style Guide, but that changed to allow these since this was > written. > * Remove phrase about "protection against a large class of lifetime mistakes" > since that appeared nowhere in the relevant discussion thread or linked CL and, > AFAICT, is not particularly true (the closest thing I found was a comment about > how captureless lambdas were not at risk for the same sorts of lifetime mistakes > in this case that capturing lambdas are -- but that's not the same thing as > implying that when you use a captureless lambda you magically gain lifetime > protections). > > I did also add the explicit mention you ask for, though now that the bit about > captureless lambdas and Bind is the last thing in the text and the text is > slightly simpler, it feels more redundant than before. Up to you. The lifetime mistakes part is true - base::Unretained and friends are required to annotate and ensure you're considering lifetimes. I think it's helpful to include when saying do not store capturing lambdas that Bind is better for these reasons. https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#n... styleguide/c++/c++11.html:484: <td>libstdc++ 4.6 has a bug where <code>std::get</code> can return an lvalue-reference when it should return an rvalue-reference. <code>base::get</code> can be used to avoid this. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/0NnCRmMY1xY">Discussion thread</a>. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/VA7Ej2IWS2w">Another discussion thread</a></td> On 2016/11/01 01:34:52, Peter Kasting wrote: > On 2016/11/01 00:49:41, danakj wrote: > > On 2016/10/31 23:46:48, Peter Kasting wrote: > > > I rewrote this as best I could based on reading the CL in question, which > had > > no > > > associated bug or discussion thread :( > > > > > > AFAICT, we're not advocating people globally use base::get over std::get, > just > > > saying it's available for certain scenarios? I tried to make this more > clear. > > > > I'm torn, consistently using base::get is nice but it's a make-work project to > > convert more code in the future so.. yeh I dont have feelings to direct > strongly > > It seems like it's a temporary workaround for libstdc++ 4.6 anyway, right? Once > we're on a newer version of that, I'd think we'd want to rip out base::get > entirely. So directing people to always use it seems ugh? right
https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#n... styleguide/c++/c++11.html:190: <td>Do not bind or store capturing lambdas outside the lifetime of the stack frame they are defined in. Captureless lambdas can be used with <code>base::Callback</code>, such as <code>base::Bind([](int j){}, i);</code>, because they offer protection against a large class of object lifetime mistakes. Don't use default captures (<code>[=]</code>, <code>[&]</code> – <a href="https://google.github.io/styleguide/cppguide.html#Lambda_expressions">Google Style Guide</a>). Lambdas are typically useful as a parameter to methods or functions that will use them immediately, such as those in <code><algorithm></code>. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/D9UnnxBnciQ">Discussion thread</a>. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/QxjsPELDYdQ">Another discussion thread</a></td> On 2016/11/01 22:22:22, danakj wrote: > On 2016/11/01 01:34:52, Peter Kasting wrote: > > On 2016/11/01 00:49:41, danakj wrote: > > > Maybe mention the 2nd thread is about interaction with bind? > > > > I rewrote this item. > > > > * Put motivation first, then caveats. > > * Soften language about default captures, since the intent was to capture the > > spirit of the Google Style Guide, but that changed to allow these since this > was > > written. > > * Remove phrase about "protection against a large class of lifetime mistakes" > > since that appeared nowhere in the relevant discussion thread or linked CL > and, > > AFAICT, is not particularly true (the closest thing I found was a comment > about > > how captureless lambdas were not at risk for the same sorts of lifetime > mistakes > > in this case that capturing lambdas are -- but that's not the same thing as > > implying that when you use a captureless lambda you magically gain lifetime > > protections). > > > > I did also add the explicit mention you ask for, though now that the bit about > > captureless lambdas and Bind is the last thing in the text and the text is > > slightly simpler, it feels more redundant than before. Up to you. > > The lifetime mistakes part is true - base::Unretained and friends are required > to annotate and ensure you're considering lifetimes. I think it's helpful to > include when saying do not store capturing lambdas that Bind is better for these > reasons. Wait, I'm confused. I think what I was saying, what you're saying, and what this text was and is saying might all be different. It seems like we have at least six different things that can happen: (1) Directly store a function (via function pointer), call it later. (2) Directly store a captureless lambda (3) Directly store a capturing lambda (4) Bind a function (5) Bind a captureless lambda (6) Bind a capturing lambda I read this paragraph as forbidding 3/6, and the phrase I deleted as saying that 5 was better than 4. Your reply sounds to me like you're saying this phrase wasn't about _lambdas_ fixing lifetime issues (compared to functions), but about using Bind in general being good (i.e. 4-6 are better than 1-3), so we want to encourage it in general, and oh BTW we allow lambdas with Bind. If so, I worry that this sentiment is a bit misplaced in this location, as it's more about Bind than about lambdas. But if you still think there's something that should go here, do you have a proposal for a phrasing that doesn't have the ambiguity above? Or should I just go ahead with the rewrite I had?
On 2016/11/01 22:50:20, Peter Kasting wrote: > https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html > File styleguide/c++/c++11.html (right): > > https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#n... > styleguide/c++/c++11.html:190: <td>Do not bind or store capturing lambdas > outside the lifetime of the stack frame they are defined in. Captureless lambdas > can be used with <code>base::Callback</code>, such as <code>base::Bind([](int > j){}, i);</code>, because they offer protection against a large class of object > lifetime mistakes. Don't use default captures (<code>[=]</code>, > <code>[&]</code> – <a > href="https://google.github.io/styleguide/cppguide.html#Lambda_expressions">Google > Style Guide</a>). Lambdas are typically useful as a parameter to methods or > functions that will use them immediately, such as those in > <code><algorithm></code>. <a > href="https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/D9UnnxBnciQ">Discussion > thread</a>. <a > href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/QxjsPELDYdQ">Another > discussion thread</a></td> > On 2016/11/01 22:22:22, danakj wrote: > > On 2016/11/01 01:34:52, Peter Kasting wrote: > > > On 2016/11/01 00:49:41, danakj wrote: > > > > Maybe mention the 2nd thread is about interaction with bind? > > > > > > I rewrote this item. > > > > > > * Put motivation first, then caveats. > > > * Soften language about default captures, since the intent was to capture > the > > > spirit of the Google Style Guide, but that changed to allow these since this > > was > > > written. > > > * Remove phrase about "protection against a large class of lifetime > mistakes" > > > since that appeared nowhere in the relevant discussion thread or linked CL > > and, > > > AFAICT, is not particularly true (the closest thing I found was a comment > > about > > > how captureless lambdas were not at risk for the same sorts of lifetime > > mistakes > > > in this case that capturing lambdas are -- but that's not the same thing as > > > implying that when you use a captureless lambda you magically gain lifetime > > > protections). > > > > > > I did also add the explicit mention you ask for, though now that the bit > about > > > captureless lambdas and Bind is the last thing in the text and the text is > > > slightly simpler, it feels more redundant than before. Up to you. > > > > The lifetime mistakes part is true - base::Unretained and friends are required > > to annotate and ensure you're considering lifetimes. I think it's helpful to > > include when saying do not store capturing lambdas that Bind is better for > these > > reasons. > > Wait, I'm confused. I think what I was saying, what you're saying, and what > this text was and is saying might all be different. > > It seems like we have at least six different things that can happen: > (1) Directly store a function (via function pointer), call it later. Fine ofc. > (2) Directly store a captureless lambda Also fine. > (3) Directly store a capturing lambda Fine if not leaving scope, because Bind does a better job wrt lifetimes so we prefer that. > (4) Bind a function Fine to use > (5) Bind a captureless lambda Fine to use > (6) Bind a capturing lambda Confusing and banned, just bind the state into Callback, for same reasons as 3 should not leave scope. > > I read this paragraph as forbidding 3/6, and the phrase I deleted as saying that > 5 was better than 4. It forbids 3 only when it leaves scope. 6 is forbidden cuz mega confusing multiple layers of capture. The sentence was meant to say that Bind/Callback deals with lifetimes better so when capturing state that leaves scope (where lifetimes become involved) use them instead. > Your reply sounds to me like you're saying this phrase wasn't about _lambdas_ > fixing lifetime issues (compared to functions), but about using Bind in general > being good (i.e. 4-6 are better than 1-3), so we want to encourage it in > general, and oh BTW we allow lambdas with Bind. 1 and 2 and 4 and 5 are all fine and use whatever looks simplest etc. 3 is fine if no lifetimes come into play (we define that in terms of scoping) and 6 is bad mixing. > If so, I worry that this sentiment is a bit misplaced in this location, as it's > more about Bind than about lambdas. > > But if you still think there's something that should go here, do you have a > proposal for a phrasing that doesn't have the ambiguity above? > > Or should I just go ahead with the rewrite I had? Sorry I repeated myself a bunch above, but I hope that makes it clear. Lambdas are typically useful as a parameter to methods or functions that will use them immediately, such as those in <code><algorithm></code>. Be careful with default captures (<code>[=]</code>, <code>[&]</code>), and do not bind or store any capturing lambdas outside the lifetime of the stack frame they are defined in, use Bind/Callback for this instead as they help get lifetimes correct. Bind may be used with captureless lambdas in the same ways as with function pointers. Something like that?
On 2016/11/01 23:00:48, danakj wrote: > On 2016/11/01 22:50:20, Peter Kasting wrote: > > https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html > > File styleguide/c++/c++11.html (right): > > > > > https://codereview.chromium.org/2466103002/diff/1/styleguide/c++/c++11.html#n... > > styleguide/c++/c++11.html:190: <td>Do not bind or store capturing lambdas > > outside the lifetime of the stack frame they are defined in. Captureless > lambdas > > can be used with <code>base::Callback</code>, such as <code>base::Bind([](int > > j){}, i);</code>, because they offer protection against a large class of > object > > lifetime mistakes. Don't use default captures (<code>[=]</code>, > > <code>[&]</code> – <a > > > href="https://google.github.io/styleguide/cppguide.html#Lambda_expressions">Google > > Style Guide</a>). Lambdas are typically useful as a parameter to methods or > > functions that will use them immediately, such as those in > > <code><algorithm></code>. <a > > > href="https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/D9UnnxBnciQ">Discussion > > thread</a>. <a > > > href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/QxjsPELDYdQ">Another > > discussion thread</a></td> > > On 2016/11/01 22:22:22, danakj wrote: > > > On 2016/11/01 01:34:52, Peter Kasting wrote: > > > > On 2016/11/01 00:49:41, danakj wrote: > > > > > Maybe mention the 2nd thread is about interaction with bind? > > > > > > > > I rewrote this item. > > > > > > > > * Put motivation first, then caveats. > > > > * Soften language about default captures, since the intent was to capture > > the > > > > spirit of the Google Style Guide, but that changed to allow these since > this > > > was > > > > written. > > > > * Remove phrase about "protection against a large class of lifetime > > mistakes" > > > > since that appeared nowhere in the relevant discussion thread or linked CL > > > and, > > > > AFAICT, is not particularly true (the closest thing I found was a comment > > > about > > > > how captureless lambdas were not at risk for the same sorts of lifetime > > > mistakes > > > > in this case that capturing lambdas are -- but that's not the same thing > as > > > > implying that when you use a captureless lambda you magically gain > lifetime > > > > protections). > > > > > > > > I did also add the explicit mention you ask for, though now that the bit > > about > > > > captureless lambdas and Bind is the last thing in the text and the text is > > > > slightly simpler, it feels more redundant than before. Up to you. > > > > > > The lifetime mistakes part is true - base::Unretained and friends are > required > > > to annotate and ensure you're considering lifetimes. I think it's helpful to > > > include when saying do not store capturing lambdas that Bind is better for > > these > > > reasons. > > > > Wait, I'm confused. I think what I was saying, what you're saying, and what > > this text was and is saying might all be different. > > > > It seems like we have at least six different things that can happen: > > (1) Directly store a function (via function pointer), call it later. > > Fine ofc. > > > (2) Directly store a captureless lambda > > Also fine. > > > (3) Directly store a capturing lambda > > Fine if not leaving scope, because Bind does a better job wrt lifetimes so we > prefer that. > > > (4) Bind a function > > Fine to use > > > (5) Bind a captureless lambda > > Fine to use > > > (6) Bind a capturing lambda > > Confusing and banned, just bind the state into Callback, for same reasons as 3 > should not leave scope. > > > > > I read this paragraph as forbidding 3/6, and the phrase I deleted as saying > that > > 5 was better than 4. > > It forbids 3 only when it leaves scope. 6 is forbidden cuz mega confusing > multiple layers of capture. > > The sentence was meant to say that Bind/Callback deals with lifetimes better so > when capturing state that leaves scope (where lifetimes become involved) use > them instead. > > > Your reply sounds to me like you're saying this phrase wasn't about _lambdas_ > > fixing lifetime issues (compared to functions), but about using Bind in > general > > being good (i.e. 4-6 are better than 1-3), so we want to encourage it in > > general, and oh BTW we allow lambdas with Bind. > > 1 and 2 and 4 and 5 are all fine and use whatever looks simplest etc. 3 is fine > if no lifetimes come into play (we define that in terms of scoping) and 6 is bad > mixing. > > > If so, I worry that this sentiment is a bit misplaced in this location, as > it's > > more about Bind than about lambdas. > > > > But if you still think there's something that should go here, do you have a > > proposal for a phrasing that doesn't have the ambiguity above? > > > > Or should I just go ahead with the rewrite I had? > > Sorry I repeated myself a bunch above, but I hope that makes it clear. > > Lambdas are typically useful as a parameter to methods or functions that will > use them immediately, such as those in <code><algorithm></code>. Be > careful with default captures (<code>[=]</code>, <code>[&]</code>), and do > not > bind or store any capturing lambdas outside the lifetime of the stack frame they > are defined in, use Bind/Callback > for this instead as they help get lifetimes correct. Bind may be used with > captureless lambdas in the same ways as > with function pointers. > > Something like that? OK, I know where you're coming from now. (BTW, with "call it later" I had intended to imply that everything on my list was for the "leaving scope" case... my attempt at clarity failed to be sufficiently clear.) I'll write something close to what you suggest.
FYI, I rewrote this in more of the vein you propose. Landing.
The CQ bit was checked by pkasting@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2466103002/#ps40001 (title: "Reword")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Styleguide cleanup part 3. * Add missing discussion thread links * Canonicalize existing links * Standardize pattern for contents of final column: notes, then Google Style Guide link, then discussion thread links * Move std::nullptr_t from a note in the language features section to its own entry in the library features section * Remove entry for "containers of move-only types" since AFAICT this isn't really a "feature" with explicit separate documentation, it never had any discussion, and usage of these is commonplace in the codebase now and doesn't need explicit encouragement/clarity * Updated/clarified some comments BUG=none TEST=none ========== to ========== Styleguide cleanup part 3. * Add missing discussion thread links * Canonicalize existing links * Standardize pattern for contents of final column: notes, then Google Style Guide link, then discussion thread links * Move std::nullptr_t from a note in the language features section to its own entry in the library features section * Remove entry for "containers of move-only types" since AFAICT this isn't really a "feature" with explicit separate documentation, it never had any discussion, and usage of these is commonplace in the codebase now and doesn't need explicit encouragement/clarity * Updated/clarified some comments BUG=none TEST=none ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Styleguide cleanup part 3. * Add missing discussion thread links * Canonicalize existing links * Standardize pattern for contents of final column: notes, then Google Style Guide link, then discussion thread links * Move std::nullptr_t from a note in the language features section to its own entry in the library features section * Remove entry for "containers of move-only types" since AFAICT this isn't really a "feature" with explicit separate documentation, it never had any discussion, and usage of these is commonplace in the codebase now and doesn't need explicit encouragement/clarity * Updated/clarified some comments BUG=none TEST=none ========== to ========== Styleguide cleanup part 3. * Add missing discussion thread links * Canonicalize existing links * Standardize pattern for contents of final column: notes, then Google Style Guide link, then discussion thread links * Move std::nullptr_t from a note in the language features section to its own entry in the library features section * Remove entry for "containers of move-only types" since AFAICT this isn't really a "feature" with explicit separate documentation, it never had any discussion, and usage of these is commonplace in the codebase now and doesn't need explicit encouragement/clarity * Updated/clarified some comments BUG=none TEST=none Committed: https://crrev.com/0cb5bd450218c4422cf3b7cb77032d1643a872e8 Cr-Commit-Position: refs/heads/master@{#429399} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0cb5bd450218c4422cf3b7cb77032d1643a872e8 Cr-Commit-Position: refs/heads/master@{#429399} |
