Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1662933002:
Allow <algorithm>. (Closed)
|
Created:
4 years, 10 months ago by Peter Kasting Modified:
4 years, 10 months ago CC:
chromium-reviews, danakj+watch_chromium.org, jbroman+cpp_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow <algorithm>.
This also makes the language consistent for the three "allow <library>" items
and removes some more-specific entries that are subsumed by the more general
items.
BUG=none
TEST=none
Committed: https://crrev.com/3adbbcc39ac3d061ce3fd34c73e27b7212dd5f8f
Cr-Commit-Position: refs/heads/master@{#373418}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Review comments #Patch Set 3 : Remove special char #Messages
Total messages: 18 (4 generated)
pkasting@chromium.org changed reviewers: + thakis@chromium.org
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (left): https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html#o... styleguide/c++/c++11.html:402: <td><code>std::move()</code></td> Can you keep this as a separate point, or keep the note about std::move in the algorithm notes?
thanks! usually we include some use of a newly allowed thing in the cl that allows it, but for <algorithm> that isn't really feasible i suppose :-) basically lg, minor comments: https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (left): https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html#o... styleguide/c++/c++11.html:402: <td><code>std::move()</code></td> On 2016/02/03 23:32:19, danakj wrote: > Can you keep this as a separate point, or keep the note about std::move in the > algorithm notes? you mean the "Moves contents of an iterator range to a different iterator. This is a counterpart of std::copy that applies std::move() to each element." note, right? (the other note is there) if so, i agree that it'd be good to keep that https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html#o... styleguide/c++/c++11.html:419: <td><code>std::round()</code>, <code>std::isnan()</code>, and others</td> i like having these function snippets; i ctrl-f for random function names on this page fairly often
https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (left): https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html#o... styleguide/c++/c++11.html:402: <td><code>std::move()</code></td> On 2016/02/03 23:46:09, Nico wrote: > On 2016/02/03 23:32:19, danakj wrote: > > Can you keep this as a separate point, or keep the note about std::move in the > > algorithm notes? > > you mean the "Moves contents of an iterator range to a different iterator. This > is a counterpart of std::copy that applies std::move() to each element." note, > right? (the other note is there) if so, i agree that it'd be good to keep that I was thinking also of the "probably don't use though" part, and link to the separate discussion thread.
https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html File styleguide/c++/c++11.html (left): https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html#o... styleguide/c++/c++11.html:402: <td><code>std::move()</code></td> On 2016/02/03 23:47:51, danakj wrote: > On 2016/02/03 23:46:09, Nico wrote: > > On 2016/02/03 23:32:19, danakj wrote: > > > Can you keep this as a separate point, or keep the note about std::move in > the > > > algorithm notes? > > > > you mean the "Moves contents of an iterator range to a different iterator. > This > > is a counterpart of std::copy that applies std::move() to each element." note, > > right? (the other note is there) if so, i agree that it'd be good to keep that > > I was thinking also of the "probably don't use though" part, So, are you saying the wording for that that I kept is insufficient, or did you just not notice that I'd preserved this? > and link to the > separate discussion thread. I intentionally didn't link this because on reading that discussion thread it added absolutely no information to this recommendation (it merely said "we decided to allow this with discouraging language"). https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html#o... styleguide/c++/c++11.html:419: <td><code>std::round()</code>, <code>std::isnan()</code>, and others</td> On 2016/02/03 23:46:09, Nico wrote: > i like having these function snippets; i ctrl-f for random function names on > this page fairly often I don't like picking a couple of functions seemingly at random from a long list. I would be totally cool with explicitly listing out all C++11-specific functions in these headers so people could ctrl-f, but listing a couple encourages people to ctrl-f, and then when they don't get any hits assume their function isn't allowed. Would you like me to add all the relevant function names to these rows?
On Wed, Feb 3, 2016 at 3:54 PM, <pkasting@chromium.org> wrote: > > https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html > File styleguide/c++/c++11.html (left): > https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html#o... > styleguide/c++/c++11.html:402 <https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html#o...>: <td><code>std::move()</code></td> > On 2016/02/03 23:47:51, danakj wrote: > > On 2016/02/03 23:46:09, Nico wrote: > > > On 2016/02/03 23:32:19, danakj wrote: > > > > Can you keep this as a separate point, or keep the note about > std::move in > > the > > > > algorithm notes? > > > > > > you mean the "Moves contents of an iterator range to a different > iterator. > > This > > > is a counterpart of std::copy that applies std::move() to each > element." note, > > > right? (the other note is there) if so, i agree that it'd be good to > keep that > > > > I was thinking also of the "probably don't use though" part, > > So, are you saying the wording for that that I kept is insufficient, or > did you just not notice that I'd preserved this? > > Oh, darn, I didn't see you saved it. I read all the other "Everything in" and the algorithm one looked the same. > > > and link to the > > separate discussion thread. > > I intentionally didn't link this because on reading that discussion > thread it added absolutely no information to this recommendation (it > merely said "we decided to allow this with discouraging language"). > > I think it's good to keep the links rather than appear to hide previous conversations. "Previous discussion thread" or something to clarify it's less meaningful if you like. > https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html#o... > styleguide/c++/c++11.html:419 <https://codereview.chromium.org/1662933002/diff/1/styleguide/c++/c++11.html#o...>: <td><code>std::round()</code>, > <code>std::isnan()</code>, and others</td> > On 2016/02/03 23:46:09, Nico wrote: > > i like having these function snippets; i ctrl-f for random function > names on > > this page fairly often > > I don't like picking a couple of functions seemingly at random from a > long list. I would be totally cool with explicitly listing out all > C++11-specific functions in these headers so people could ctrl-f, but > listing a couple encourages people to ctrl-f, and then when they don't > get any hits assume their function isn't allowed. > > Would you like me to add all the relevant function names to these rows? > > I also like having a few of the most common functions :) > https://codereview.chromium.org/1662933002/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/02/03 23:58:24, danakj wrote: > > > and link to the > > > separate discussion thread. > > > > I intentionally didn't link this because on reading that discussion > > thread it added absolutely no information to this recommendation (it > > merely said "we decided to allow this with discouraging language"). > > > > I think it's good to keep the links rather than appear to hide previous > conversations. "Previous discussion thread" or something to clarify it's > less meaningful if you like. I can try to add something, I guess. It would have been nicer had said discussion thread provided any actual information or guidance whatsoever. For example, I have no idea why we even have this recommendation text, because the thread never justified its addition. > > Would you like me to add all the relevant function names to these rows? > > > > I also like having a few of the most common functions :) Is that a way of saying "I am opposed to adding the complete list"? I'm asking in part because we need to be consistent across items here. If we list some things for <cmath>, we should list some things for e.g. <algorithm> as well. I'm still very concerned about encouraging people to ctrl-f in this document as long as our stance on C++11 library features is generally "not allowed except where noted". I could post a version with the full list added to get an idea of how ugly it actually looks.
On Wed, Feb 3, 2016 at 4:06 PM, <pkasting@chromium.org> wrote: > On 2016/02/03 23:58:24, danakj wrote: > > > > and link to the > > > > separate discussion thread. > > > > > > I intentionally didn't link this because on reading that discussion > > > thread it added absolutely no information to this recommendation (it > > > merely said "we decided to allow this with discouraging language"). > > > > > > I think it's good to keep the links rather than appear to hide previous > > conversations. "Previous discussion thread" or something to clarify it's > > less meaningful if you like. > > I can try to add something, I guess. It would have been nicer had said > discussion thread provided any actual information or guidance whatsoever. For > example, I have no idea why we even have this recommendation text, because the > thread never justified its addition. > > Eh, just having a link doesn't seem that problematic? People may necro and add to the thread over time too. > > > > Would you like me to add all the relevant function names to these rows? > > > > > > I also like having a few of the most common functions :) > > Is that a way of saying "I am opposed to adding the complete list"? > > I'm asking in part because we need to be consistent across items here. If we > list some things for <cmath>, we should list some things for e.g. <algorithm> as > well. > > I'm still very concerned about encouraging people to ctrl-f in this document as > long as our stance on C++11 library features is generally "not allowed except > where noted". I could post a version with the full list added to get an idea of > how ugly it actually looks. > > Sure. Control-f is a shortcut and if it fails maybe you fall back to figuring out the header and looking for that. It's okay to not list everything IMO, and I was concerned that sounded like it would be a wall of text. If it's reasonable, I don't hate having them all either. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
New snap up: * Lists all functions in <algorithms> and a significant subset for <cmath> and <type_traits> (these were way too long to list everything) * Adds more detail and second discussion link for range-base move(). I tried to justify the recommendation as well, to the best of my understanding. * Notes that not _all_ of type_traits is allowed, since aligned storage features are still in the TBD section
LGTM
The CQ bit was checked by pkasting@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1662933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1662933002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Allow <algorithm>. This also makes the language consistent for the three "allow <library>" items and removes some more-specific entries that are subsumed by the more general items. BUG=none TEST=none ========== to ========== Allow <algorithm>. This also makes the language consistent for the three "allow <library>" items and removes some more-specific entries that are subsumed by the more general items. BUG=none TEST=none Committed: https://crrev.com/3adbbcc39ac3d061ce3fd34c73e27b7212dd5f8f Cr-Commit-Position: refs/heads/master@{#373418} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3adbbcc39ac3d061ce3fd34c73e27b7212dd5f8f Cr-Commit-Position: refs/heads/master@{#373418}
Message was sent while issue was closed.
lgtm, thanks for the great follow-up :-) |
