Closed Bug 1465585 Opened 6 years ago Closed 6 years ago

Switch mozilla::Move to std::move.

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(5 files)

Bug 1465585: Automatic replacements.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1465585: Remove 'using mozilla::Move;'.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1465585: Manual fixups, plus removal of mozilla::Move.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1465585: Disable stl wrapping in some places under toolkit/mozapps/update.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1465585: Don't error on pessimizing-move and self-move, for now.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
No description provided.
Version: Version 3 → Trunk
Assignee: nobody → emilio
Comment on attachment 8982061 [details] Bug 1465585: Automatic replacements. https://reviewboard.mozilla.org/r/248098/#review254418 rs=me; I'm not going to click through 14 pages in mozreview, and the sed/perl script seems reasonable enough. And rs=me on any re-running the script needs prior to landing, of course.
Attachment #8982061 - Flags: review?(nfroyd) → review+
Comment on attachment 8982062 [details] Bug 1465585: Remove 'using mozilla::Move;'. https://reviewboard.mozilla.org/r/248100/#review254420 \o/
Attachment #8982062 - Flags: review?(nfroyd) → review+
Comment on attachment 8982063 [details] Bug 1465585: Manual fixups, plus removal of mozilla::Move. https://reviewboard.mozilla.org/r/248102/#review254422 ::: editor/libeditor/PlaceholderTransaction.cpp:26 (Diff revision 3) > nsAtom* aName, > Maybe<SelectionState>&& aSelState) > : mEditorBase(&aEditorBase) > , mForwarding(nullptr) > , mCompositionTransaction(nullptr) > - , mStartSel(*Move(aSelState)) > + , mStartSel(*std::move(aSelState)) ...code that passes `Maybe`, but unconditionally derefs it, great. ::: gfx/tests/gtest/MockWidget.h:61 (Diff revision 3) > nsNativeWidget aNativeParent, > const DesktopIntRect& aRect, > nsWidgetInitData* aInitData = nullptr) override { return NS_OK; } > virtual void Show(bool aState) override {} > virtual bool IsVisible() const override { return true; } > - virtual void std::move(double aX, double aY) override {} > + virtual void Move(double aX, double aY) override {} Whoops! ::: mfbt/Move.h:14 (Diff revision 3) > #ifndef mozilla_Move_h > #define mozilla_Move_h > > #include "mozilla/TypeTraits.h" > > +#include <utility> Aha, this is what makes us not have to adjust a bunch of headers, very nice. ::: toolkit/components/protobuf/src/google/protobuf/map_entry_lite.h:63 (Diff revision 3) > // MoveHelper::Move is used to set *dest. It copies *src, or moves it (in > // the C++11 sense), or swaps it. *src is left in a sane state for > // subsequent destruction, but shouldn't be used for anything. > template <bool is_enum, bool is_message, bool is_stringlike, typename T> > struct MoveHelper { // primitives > - static void std::move(T* src, T* dest) { *dest = *src; } > + static void Move(T* src, T* dest) { *dest = *src; } Actually, it would probably be good if the patches didn't do the substitution and then undo it. I understand that it does make things substantially easier to do it this way, though...WDYT? ::: toolkit/components/protobuf/src/google/protobuf/map_entry_lite.h:83 (Diff revision 3) > > template <typename T> > struct MoveHelper<false, false, true, T> { // strings and similar > - static void std::move(T* src, T* dest) { > + static void Move(T* src, T* dest) { > #if __cplusplus >= 201103L > - *dest = std::move(*src); > + *dest = Move(*src); This seems like it should indeed be `std::move`, since it's in a C++11 conditional. The `std::move` is also the original code.
Attachment #8982063 - Flags: review?(nfroyd) → review+
Comment on attachment 8982075 [details] Bug 1465585: Don't error on pessimizing-move and self-move, for now. https://reviewboard.mozilla.org/r/248118/#review254426
Attachment #8982075 - Flags: review?(nfroyd) → review+
Comment on attachment 8982074 [details] Bug 1465585: Disable stl wrapping in some places under toolkit/mozapps/update. https://reviewboard.mozilla.org/r/248116/#review254432 I'm going to pass this review to glandium, as he understands the setup a little better than I do, and may be able to provide more useful feedback. Have you found that you need to halt inclusion on Windows and non-Windows? Windows platforms were always the problem for `TypeTraits.h`; maybe the circumstances are different for `Move`, though? ::: config/gcc-stl-wrapper.template.h:35 (Diff revision 1) > +// Don't include mozalloc for <utility> or <type_traits>, since we have > +// compilation units with NS_NO_XPCOM that want them, and mozalloc is not needed > +// there anyway. > +#ifndef moz_dont_include_mozalloc_for_utility > +# define moz_dont_include_mozalloc_for_utility > +#endif > +#ifndef moz_dont_include_mozalloc_for_type_traits > +# define moz_dont_include_mozalloc_for_type_traits > +#endif I am not clear that this is actually the right thing. We're wrapping the STL headers so we can ensure the actual headers pick up our global definitions of `operator new` and company. But, as you can see, we do have exceptions for some headers. I think it's correct that `type_traits` doesn't need to be wrapped, as there really shouldn't be memory-allocating bits in that header. But I'm less certain about `<utility>`, because things like smart pointers are contained therein. (`<utility>` is what has held up getting rid of `TypeTraits.h`, probably for similar reasons as what you've discovered doing this bug. So it'd be nice to find the correct solution for all time here.)
Attachment #8982074 - Flags: review?(nfroyd) → review?(mh+mozilla)
(In reply to Nathan Froyd [:froydnj] from comment #17) > Comment on attachment 8982063 [details] > Bug 1465585: Manual fixups, plus removal of mozilla::Move. > > https://reviewboard.mozilla.org/r/248102/#review254422 > > ::: editor/libeditor/PlaceholderTransaction.cpp:26 > (Diff revision 3) > > nsAtom* aName, > > Maybe<SelectionState>&& aSelState) > > : mEditorBase(&aEditorBase) > > , mForwarding(nullptr) > > , mCompositionTransaction(nullptr) > > - , mStartSel(*Move(aSelState)) > > + , mStartSel(*std::move(aSelState)) > > ...code that passes `Maybe`, but unconditionally derefs it, great. > > ::: gfx/tests/gtest/MockWidget.h:61 > (Diff revision 3) > > nsNativeWidget aNativeParent, > > const DesktopIntRect& aRect, > > nsWidgetInitData* aInitData = nullptr) override { return NS_OK; } > > virtual void Show(bool aState) override {} > > virtual bool IsVisible() const override { return true; } > > - virtual void std::move(double aX, double aY) override {} > > + virtual void Move(double aX, double aY) override {} > > Whoops! > > ::: mfbt/Move.h:14 > (Diff revision 3) > > #ifndef mozilla_Move_h > > #define mozilla_Move_h > > > > #include "mozilla/TypeTraits.h" > > > > +#include <utility> > > Aha, this is what makes us not have to adjust a bunch of headers, very nice. > > ::: toolkit/components/protobuf/src/google/protobuf/map_entry_lite.h:63 > (Diff revision 3) > > // MoveHelper::Move is used to set *dest. It copies *src, or moves it (in > > // the C++11 sense), or swaps it. *src is left in a sane state for > > // subsequent destruction, but shouldn't be used for anything. > > template <bool is_enum, bool is_message, bool is_stringlike, typename T> > > struct MoveHelper { // primitives > > - static void std::move(T* src, T* dest) { *dest = *src; } > > + static void Move(T* src, T* dest) { *dest = *src; } > > Actually, it would probably be good if the patches didn't do the > substitution and then undo it. I understand that it does make things > substantially easier to do it this way, though...WDYT? Yeah, I'll squash those bits before landing. > ::: toolkit/components/protobuf/src/google/protobuf/map_entry_lite.h:83 > (Diff revision 3) > > > > template <typename T> > > struct MoveHelper<false, false, true, T> { // strings and similar > > - static void std::move(T* src, T* dest) { > > + static void Move(T* src, T* dest) { > > #if __cplusplus >= 201103L > > - *dest = std::move(*src); > > + *dest = Move(*src); > > This seems like it should indeed be `std::move`, since it's in a C++11 > conditional. The `std::move` is also the original code. whoops, will fix, nice catch!
Comment on attachment 8982074 [details] Bug 1465585: Disable stl wrapping in some places under toolkit/mozapps/update. https://reviewboard.mozilla.org/r/248116/#review254572 None of these end up i
Attachment #8982074 - Flags: review?(mh+mozilla) → review+
mozreview ate my message O_o None of these end up in libxul, so that's fine.
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/73a2ea4007a0 Disable stl wrapping in some places under toolkit/mozapps/update. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/6979fe6c19b1 Don't error on pessimizing-move and self-move, for now. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/b54db6622358 Switch from mozilla::Move to std::move. r=froydnj
On Mac there are now loads of these warnings in the console FWIW: dom/canvas/Unified_cpp_dom_canvas0.cpp:47: dom/canvas/ImageBitmap.cpp:807:10: warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move] return std::move(result); ^ dom/canvas/ImageBitmap.cpp:807:10: note: remove std::move call here return std::move(result); ^~~~~~~~~~ ~ warning: moving a local object in a return statement prevents copy elision
Also I got a build error while building layout/style/test/ListCSSProperties.cpp Move.h:222:14: error: no type named 'move' in namespace 'std' That needed a clobber to resolve it.
(In reply to Jonathan Watt [:jwatt] from comment #30) > On Mac there are now loads of these warnings in the console FWIW: > > dom/canvas/Unified_cpp_dom_canvas0.cpp:47: > dom/canvas/ImageBitmap.cpp:807:10: warning: moving a local object in a > return statement prevents copy elision [-Wpessimizing-move] > return std::move(result); > ^ > dom/canvas/ImageBitmap.cpp:807:10: note: remove std::move call here > return std::move(result); > ^~~~~~~~~~ ~ > warning: moving a local object in a return statement prevents copy elision Yeah, that is expected, they'll go away in bug 1465060.
Depends on: 1270217
Blocks: 1466168
(In reply to Mike Hommey [:glandium] from comment #32) > Yeah, big jump in warnings: > https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-inbound, > 1515387,1,2&selected=mozilla-inbound,1515387,343554,480097001 -Wpessimizing-move: 153 occurrences -Wreturn-std-move: 14 occurrences -Wself-move: 2 occurrences
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: