Closed Bug 1465585 Opened 6 years ago Closed 6 years ago

Switch mozilla::Move to std::move.

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(5 files)

      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.