Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2449973006:
Styleguide cleanup part 1. (Closed)
|
Created:
4 years, 1 month ago by Peter Kasting Modified:
4 years, 1 month ago Reviewers:
danakj CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStyleguide cleanup part 1.
* Most documentation links already went to cppreference.com; shift over a few more.
* Add some missing documentation and Google Style Guide links.
* Fix broken documentation links.
* Make documentation link titles match landing page titles.
* Link directly to specific subsections of documentation pages in a few cases.
* Minor edits for clarity, consistency, or typo fixes.
BUG=none
TEST=none
Committed: https://crrev.com/7f0693b36c4efdd6f82a8ecc4391364f36ceba50
Cr-Commit-Position: refs/heads/master@{#428788}
Patch Set 1 #Patch Set 2 : Remove accidentally-inserted sentence. #
Total comments: 7
Patch Set 3 : Review comments #Messages
Total messages: 15 (5 generated)
Description was changed from ========== Styleguide cleanup part 1. BUG=none TEST=none ========== to ========== Styleguide cleanup part 1. * Most documentation links already went to cppreference.com; shift over a few more. * Add some missing documentation and Google Style Guide links. * Fix broken documentation links. * Make documentation link titles match landing page titles. * Link directly to specific subsections of documentation pages in a few cases. * Minor edits for clarity, consistency, or typo fixes. BUG=none TEST=none ==========
pkasting@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2449973006/diff/20001/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/2449973006/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:89: <td><a href="http://en.cppreference.com/w/cpp/language/type_alias">Type alias, alias template</a></td> I think calling out the word "using" was useful here https://codereview.chromium.org/2449973006/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:310: <td><a href="http://en.cppreference.com/w/cpp/language/parameter_pack#Template_parameter_list">Template parameter list</a></td> Why is this going to this specific use of ... instead of the concept of variadic templates in general? I think the old URL was better probably https://codereview.chromium.org/2449973006/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:582: <td>Banned in the <a href="https://google.github.io/styleguide/cppguide.html#C++11">Google Style Guide</a>. May be used in Chromium only with explicit approval from <code>styleguide/c++/OWNERS</code>. <a href="https://groups.google.com/a/chromium.org/d/topic/cxx/gowclr2LPQA/discussion">Discussion Thread</a></td> Kinda like the harder wording that starts with "Banned in chromium" instead of "May be used in Chromium"
Other than that this looks really nice, thanks
https://codereview.chromium.org/2449973006/diff/20001/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/2449973006/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:89: <td><a href="http://en.cppreference.com/w/cpp/language/type_alias">Type alias, alias template</a></td> On 2016/10/29 01:02:47, danakj wrote: > I think calling out the word "using" was useful here I'm happy to call it out in the other sections, but as the title of the documentation link I don't want to because that's not what the page it links to is actually called. For example, I could change the feature name to "Type Aliases ("using" Instead Of "typedef")". https://codereview.chromium.org/2449973006/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:310: <td><a href="http://en.cppreference.com/w/cpp/language/parameter_pack#Template_parameter_list">Template parameter list</a></td> On 2016/10/29 01:02:47, danakj wrote: > Why is this going to this specific use of ... instead of the concept of variadic > templates in general? I think the old URL was better probably Will revert. https://codereview.chromium.org/2449973006/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:582: <td>Banned in the <a href="https://google.github.io/styleguide/cppguide.html#C++11">Google Style Guide</a>. May be used in Chromium only with explicit approval from <code>styleguide/c++/OWNERS</code>. <a href="https://groups.google.com/a/chromium.org/d/topic/cxx/gowclr2LPQA/discussion">Discussion Thread</a></td> On 2016/10/29 01:02:47, danakj wrote: > Kinda like the harder wording that starts with "Banned in chromium" instead of > "May be used in Chromium" The reason I changed it was that if it's banned in the Google style guide, it's banned by default, so there's no reason to mention anything else. But we've carved out an exception in Chromium where this is allowed in rare cases. So the whole point of that sentence is to describe the exception. Starting with "banned" felt like it kind of missed the point. (Also, if it was in a "banned" section, and then said "banned" twice more in the description, except this is one of the only things we actually allow [rarely], it felt like "methinks thou dost protest too much", and the language and the actual status of the feature were at odds.) (Or maybe I'm just protesting too much.)
PTAL https://codereview.chromium.org/2449973006/diff/20001/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/2449973006/diff/20001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:582: <td>Banned in the <a href="https://google.github.io/styleguide/cppguide.html#C++11">Google Style Guide</a>. May be used in Chromium only with explicit approval from <code>styleguide/c++/OWNERS</code>. <a href="https://groups.google.com/a/chromium.org/d/topic/cxx/gowclr2LPQA/discussion">Discussion Thread</a></td> I changed the wording just slightly to "May only be used... with" instead of "May be used... only with". That sounds a little more restrictive upfront.
KGTM
LGTM too
The CQ bit was checked by pkasting@chromium.org
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 1. * Most documentation links already went to cppreference.com; shift over a few more. * Add some missing documentation and Google Style Guide links. * Fix broken documentation links. * Make documentation link titles match landing page titles. * Link directly to specific subsections of documentation pages in a few cases. * Minor edits for clarity, consistency, or typo fixes. BUG=none TEST=none ========== to ========== Styleguide cleanup part 1. * Most documentation links already went to cppreference.com; shift over a few more. * Add some missing documentation and Google Style Guide links. * Fix broken documentation links. * Make documentation link titles match landing page titles. * Link directly to specific subsections of documentation pages in a few cases. * Minor edits for clarity, consistency, or typo fixes. 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 1. * Most documentation links already went to cppreference.com; shift over a few more. * Add some missing documentation and Google Style Guide links. * Fix broken documentation links. * Make documentation link titles match landing page titles. * Link directly to specific subsections of documentation pages in a few cases. * Minor edits for clarity, consistency, or typo fixes. BUG=none TEST=none ========== to ========== Styleguide cleanup part 1. * Most documentation links already went to cppreference.com; shift over a few more. * Add some missing documentation and Google Style Guide links. * Fix broken documentation links. * Make documentation link titles match landing page titles. * Link directly to specific subsections of documentation pages in a few cases. * Minor edits for clarity, consistency, or typo fixes. BUG=none TEST=none Committed: https://crrev.com/7f0693b36c4efdd6f82a8ecc4391364f36ceba50 Cr-Commit-Position: refs/heads/master@{#428788} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7f0693b36c4efdd6f82a8ecc4391364f36ceba50 Cr-Commit-Position: refs/heads/master@{#428788} |
