Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1499293002:
Move Rvalue references to the allowed section of the C++11 styleguide. (Closed)
DescriptionMove Rvalue references to the allowed section of the C++11 styleguide.
They were in the library area, but they are a language feature, so
moved to the language area.
R=Nico
Committed: https://crrev.com/2c5aee92a0e3a40958f69500b563eb590c0df45f
Cr-Commit-Position: refs/heads/master@{#363568}
Patch Set 1 #Patch Set 2 : stylervalues: . #Patch Set 3 : stylervalues: linky #
Total comments: 2
Patch Set 4 : stylervalues: rebase-and-bugs #Patch Set 5 : stylervalues: 3rdbug #
Total comments: 2
Patch Set 6 : stylervalues: typo #Patch Set 7 : stylervalues: template-fix #Messages
Total messages: 29 (11 generated)
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Oh this points to the old discussion thread. I'll add the new one.
On 2015/12/04 23:40:08, danakj (behind on reviews) wrote: > Oh this points to the old discussion thread. I'll add the new one. Done.
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1499293002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1499293002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
ping
https://codereview.chromium.org/1499293002/diff/40001/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/1499293002/diff/40001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:254: <td>As per the <a href="https://google.github.io/styleguide/cppguide.html#Rvalue_references">Google style guide</a>: Only use these to define move constructors and move assignment operators, and for perfect forwarding.<br/>Most classes should not be copyable, even if movable. Continue to use DISALLOW_COPY_AND_ASSIGN (or DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND if needed) in most cases. <a href="https://groups.google.com/a/chromium.org/d/topic/chromium-dev/UnRaORb4TSw">Discussion thread</a> and <a href="https://groups.google.com/a/chromium.org/d/topic/cxx/Q526tkruXpM">discussion thread</a>.</td> Should we list the MSVC bugs we know about here too?
https://codereview.chromium.org/1499293002/diff/40001/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/1499293002/diff/40001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:254: <td>As per the <a href="https://google.github.io/styleguide/cppguide.html#Rvalue_references">Google style guide</a>: Only use these to define move constructors and move assignment operators, and for perfect forwarding.<br/>Most classes should not be copyable, even if movable. Continue to use DISALLOW_COPY_AND_ASSIGN (or DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND if needed) in most cases. <a href="https://groups.google.com/a/chromium.org/d/topic/chromium-dev/UnRaORb4TSw">Discussion thread</a> and <a href="https://groups.google.com/a/chromium.org/d/topic/cxx/Q526tkruXpM">discussion thread</a>.</td> On 2015/12/07 19:00:35, Nico wrote: > Should we list the MSVC bugs we know about here too? Done! (I only remember 2, do you remember more?)
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1499293002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1499293002/60001
(from blaikie, for that 3rd bug:
MSVC just never created implicit move ops.
Easiest example:
struct foo {
std::unique_ptr<int> u;
};
int main() {
foo f;
foo g = std::move(f);
}
That wouldn't compile in MSVC 2013, if I recall correctly.)
errr just realized i didn't hit send in this tab yet. This is what the previous comment refers to. lgtm, thanks. Another bug I know about that I think we didn't run into yet is that MSVC2013 won't synthesize move ctor/assignment operator -- if you need it, you must explicitly provide it. (And you can't just =default it either.)
On 2015/12/07 19:26:48, Nico wrote: > errr just realized i didn't hit send in this tab yet. This is what the previous > comment refers to. > > lgtm, thanks. > > Another bug I know about that I think we didn't run into yet is that MSVC2013 > won't synthesize move ctor/assignment operator -- if you need it, you must > explicitly provide it. (And you can't just =default it either.) Ah, yes. Added!
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1499293002/#ps80001 (title: "stylervalues: 3rdbug")
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1499293002/#ps100001 (title: "stylervalues: typo")
https://codereview.chromium.org/1499293002/diff/80001/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/1499293002/diff/80001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:260: <li>The compile does not create default move constructors, either implicitly or with the =default keyword. You must provide such constructors explicitly to create them.</li> s/the compile/the compiler/
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1499293002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1499293002/80001
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1499293002/#ps120001 (title: "stylervalues: template-fix")
https://codereview.chromium.org/1499293002/diff/80001/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/1499293002/diff/80001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:260: <li>The compile does not create default move constructors, either implicitly or with the =default keyword. You must provide such constructors explicitly to create them.</li> On 2015/12/07 19:34:46, Nico wrote: > s/the compile/the compiler/ Yaaa oops. I typod in my patchset description about this typo too, haha. Also, I noticed I didnt use < for the template <> stuff, which doesn't work of course. Fixed that.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1499293002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1499293002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Move Rvalue references to the allowed section of the C++11 styleguide. They were in the library area, but they are a language feature, so moved to the language area. R=Nico ========== to ========== Move Rvalue references to the allowed section of the C++11 styleguide. They were in the library area, but they are a language feature, so moved to the language area. R=Nico Committed: https://crrev.com/2c5aee92a0e3a40958f69500b563eb590c0df45f Cr-Commit-Position: refs/heads/master@{#363568} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/2c5aee92a0e3a40958f69500b563eb590c0df45f Cr-Commit-Position: refs/heads/master@{#363568} |
