944176 - Scoped pointer types <mfbt/Scoped.h> should support move construction and move assignment

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jim Blandy :jimb]

Types derived from mozilla::Scoped, defined in mfbt/Scoped.h, should support move construction and move assignment. This would make Vector<ScopedBlahPtr<Blorg> > work --- right now it can't generate code to resize the vector's internal array, because it can't move the Scoped elements from the old array to the new array.

(In fact, as reported in bug 944173, the compiler ends up generating code to make *two* Scoped instances point to the same resource --- so Vector<ScopedBlahPtr<...>> compiles, and then crashes. But fixing that bug would only change the run-time crash to a compile-time error. Fixing this bug would actually make it work.)

Comment 1

[Jim Blandy :jimb]

Attachment: Implement move construction and move assignment for mozilla::Scoped derivatives.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8339606 [details] [diff] [review]
Implement move construction and move assignment for mozilla::Scoped derivatives.

Review of attachment 8339606 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. I'll only mark f+ because I am not a peer of mfbt/, plus I haven't used Move() in my own code yet.
Does that build with VC++, though?

::: mfbt/Scoped.h
@@ +187,5 @@
>  template<typename Type>                                        \
>  struct name : public mozilla::Scoped<Traits<Type> >            \
>  {                                                              \
>      typedef mozilla::Scoped<Traits<Type> > Super;              \
>      typedef typename Super::Resource Resource;                 \

I don't think we're using Resource anymore, are we?

@@ +190,5 @@
>      typedef mozilla::Scoped<Traits<Type> > Super;              \
>      typedef typename Super::Resource Resource;                 \
> +    template <typename T>                                      \
> +    name& operator=(T&& ptr) {                                 \
> +      Super::operator=(mozilla::Forward<T>(ptr));              \

Ah, well, there go our build time improvements...

Comment 3

[Jim Blandy :jimb]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=e7361186b3de

Comment 4

[Jim Blandy :jimb]

Try, Builds only, with 'Resource' deleted:
https://tbpl.mozilla.org/?tree=Try&rev=791fecf5550a

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8339606 [details] [diff] [review]
Implement move construction and move assignment for mozilla::Scoped derivatives.

Review of attachment 8339606 [details] [diff] [review]:
-----------------------------------------------------------------

Don't land this without a (separate) patch landing at the same time that uses all the various nooks and crannies of the new stuff.  The template-copying stuff below needs some real-world testing of my understandings, or at least double-checking of them as those are corners I try not to spend too much time in if I can help it.

Hmm, actually the move/assignment template bits are probably finicky enough it's best to be safe and do another pass on it.  Should be quick, at least!

::: mfbt/Scoped.h
@@ +92,5 @@
>        MOZ_GUARD_OBJECT_NOTIFIER_INIT;
>      }
> +
> +    /* Move constructor. */
> +    explicit Scoped(Scoped<Traits>&& v

Scoped&& v is a little more readable.

@@ +162,5 @@
> +    Scoped<Traits>& operator=(Scoped<Traits>&& rhs) {
> +      Traits::release(value);
> +      value = Move(rhs.value);
> +      rhs.value = Traits::empty();
> +      return *this;

Leaning on the built-in destructor is kind of nice for reducing repetition, I think.  Also assert against self-assignment.

MOZ_ASSERT(this != &rhs, "self-assignment not allowed");
this->~Scoped();
value = Move(rhs.value);
rhs.value = Traits::empty();
return *this;

@@ +190,5 @@
>      typedef mozilla::Scoped<Traits<Type> > Super;              \
>      typedef typename Super::Resource Resource;                 \
> +    template <typename T>                                      \
> +    name& operator=(T&& ptr) {                                 \
> +      Super::operator=(mozilla::Forward<T>(ptr));              \

s/ptr/value/, or s/ptr/t/, or something.  This isn't really a pointer any more, if it ever was.  :-)  Same for the other constructors.

@@ +196,5 @@
>      }                                                          \
>      explicit name(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM)        \
>        : Super(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_TO_PARENT)  \
>      {}                                                         \
> +    template <typename T>                                      \

template<typename T>

@@ +197,5 @@
>      explicit name(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM)        \
>        : Super(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_TO_PARENT)  \
>      {}                                                         \
> +    template <typename T>                                      \
> +    explicit name(T&& ptr MOZ_GUARD_OBJECT_NOTIFIER_PARAM)     \

Put the macro on the next line for clarity.

@@ +204,3 @@
>      {}                                                         \
>    private:                                                     \
>      explicit name(name& source) MOZ_DELETE;                    \

I think you need to copy this entire move-constructor-looking thing, into a second, fully separate instance that's not templated, like so:

    template <typename T> \
    explicit name(name&& source \
                  MOZ_GUARD_OBJECT_NOTIFIER_PARAM) \
      : Super(mozilla::MOve(source) \
              MOZ_GUARD_OBJECT_NOTIFIER_PARAM_TO_PARENT) \
    {} \

The issue is that move constructors are, per spec, *non-template* functions.  If you have only a template version, that won't prevent the default move constructor from being generated.  (The copy-ctor deletion below might, I don't know the semantics of the whens offhand, or if our compilers implement them correctly.)

Come to think of it, I think you need the exact same sort of thing for the move-assignment operator bit as well -- duplicating it into a non-template function taking |name&& n|, that passes along |mozilla::Move(n)| to the superclass.

Comment 6

[Jim Blandy :jimb]

Seems like there are some B2G ICS build failures, too.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8339606 [details] [diff] [review]
Implement move construction and move assignment for mozilla::Scoped derivatives.

Review of attachment 8339606 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Scoped.h
@@ +204,3 @@
>      {}                                                         \
>    private:                                                     \
>      explicit name(name& source) MOZ_DELETE;                    \

So, let's retry saying this.

You have a templated move-constructor declared and defined.  That's fine.  The issue is that the canonical move constructor that C++ will default-generate for you, and that will be picked if selected, *must* be not a template function.  So I think you need to add

  explicit name(name&& source \
                MOZ_GUARD_OBJECT_NOTIFIER_PARAM) \
    : Super(mozilla::MOve(source) \
            MOZ_GUARD_OBJECT_NOTIFIER_PARAM_TO_PARENT) \
  {} \

into this overall macro, so that a call that attempts to use |name|'s move constructor, will properly find the one we define (and not pick up a possibly-compiler-generated version).

All the same is also true for the move-assignment operator.  You need a non-template version of that, too, so that you don't get the compiler-generated version.

Comment 8

[Jim Blandy :jimb]

You know, I don't think there's any need whatsoever for that move constructor to be templated. I think I wrote it after a bout of perfect forwarding, and was just on autopilot.

Revised patch on its way.

Comment 9

[Jim Blandy :jimb]

Attachment: In mozilla::Scoped, don't gratuitously repeat template arguments within the class template itself.

Comment 10

[Jim Blandy :jimb]

Attachment: Implement move construction and move assignment for mozilla::Scoped derivatives.

Comment 11

[Jim Blandy :jimb]

New try push:
https://tbpl.mozilla.org/?tree=Try&rev=a54eb4317227

Comment 12

[Jim Blandy :jimb]

Attachment: Implement move construction and move assignment for mozilla::Scoped derivatives.

Boo, botched the guard objects. Try cancelled. This one has actually been used, tests have passed, etc.

Comment 13

[Jim Blandy :jimb]

This time for sure!!
https://tbpl.mozilla.org/?tree=Try&rev=414abdffe868

Comment 14

[Jim Blandy :jimb]

The try push generally looks clean; there are two B2G build failures, but those look like infrastructure issues to me:

21:48:20     INFO -  repo has been initialized in /builds/slave/b2g_try_emu-jb-d_dep-000000000/build
21:48:29     INFO -  fatal: Unable to create '/builds/slave/b2g_try_emu-jb-d_dep-000000000/build/.repo/projects/gaia.git/refs/remotes/mozillaorg/v1.2.lock': File exists.
21:48:29     INFO -  If no other git process is currently running, this probably means a
21:48:29     INFO -  git process crashed in this repository earlier. Make sure no other git
21:48:29     INFO -  process is running and remove the file manually to continue.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 8394566 [details] [diff] [review]
Implement move construction and move assignment for mozilla::Scoped derivatives.

Review of attachment 8394566 [details] [diff] [review]:
-----------------------------------------------------------------

rhs is a good/better name than ptr, nicely chosen.

::: mfbt/Scoped.h
@@ +205,2 @@
>                    MOZ_GUARD_OBJECT_NOTIFIER_PARAM)             \
> +      : Super(rhs MOZ_GUARD_OBJECT_NOTIFIER_PARAM_TO_PARENT)   \

Make this

  : Super(rhs           \
          MOZ_GUARD_OBJECT_NOTIFIER_PARAM_TO_PARENT) \

so that the two don't visually blend together so much.

@@ +207,5 @@
> +    {}                                                         \
> +    explicit name(name&& rhs                                   \
> +                  MOZ_GUARD_OBJECT_NOTIFIER_PARAM)             \
> +      : Super(Move(rhs)                                        \
> +              MOZ_GUARD_OBJECT_NOTIFIER_PARAM_TO_PARENT)       \

These don't blend together, isn't it nicer to read?  :-)

Comment 16

[Jim Blandy :jimb]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15)
> rhs is a good/better name than ptr, nicely chosen.

:)

> These don't blend together, isn't it nicer to read?  :-)

It definitely is. Thanks for the careful re-reading; I'd intended to do it that way.

Comment 17

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a673af0b3f22
https://hg.mozilla.org/integration/mozilla-inbound/rev/012fea76225f

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a673af0b3f22
https://hg.mozilla.org/mozilla-central/rev/012fea76225f