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

RESOLVED FIXED in mozilla31

Status

()

Core
MFBT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

unspecified
mozilla31
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.)
(Assignee)

Comment 1

4 years ago
Created attachment 8339606 [details] [diff] [review]
Implement move construction and move assignment for mozilla::Scoped derivatives.
Attachment #8339606 - Flags: review?(dteller)
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...
Attachment #8339606 - Flags: review?(jwalden+bmo)
Attachment #8339606 - Flags: review?(dteller)
Attachment #8339606 - Flags: feedback+
(Assignee)

Comment 3

4 years ago
Try push: https://tbpl.mozilla.org/?tree=Try&rev=e7361186b3de
(Assignee)

Comment 4

4 years ago
Try, Builds only, with 'Resource' deleted:
https://tbpl.mozilla.org/?tree=Try&rev=791fecf5550a
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.
Attachment #8339606 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 6

4 years ago
Seems like there are some B2G ICS build failures, too.
(Assignee)

Updated

4 years ago
Blocks: 960786
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.
(Assignee)

Comment 8

4 years ago
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.
(Assignee)

Comment 9

4 years ago
Created attachment 8394550 [details] [diff] [review]
In mozilla::Scoped, don't gratuitously repeat template arguments within the class template itself.
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Attachment #8394550 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 10

4 years ago
Created attachment 8394552 [details] [diff] [review]
Implement move construction and move assignment for mozilla::Scoped derivatives.
Attachment #8339606 - Attachment is obsolete: true
Attachment #8394552 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 11

4 years ago
New try push:
https://tbpl.mozilla.org/?tree=Try&rev=a54eb4317227
(Assignee)

Comment 12

4 years ago
Created attachment 8394566 [details] [diff] [review]
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.
Attachment #8394552 - Attachment is obsolete: true
Attachment #8394552 - Flags: review?(jwalden+bmo)
Attachment #8394566 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 13

4 years ago
This time for sure!!
https://tbpl.mozilla.org/?tree=Try&rev=414abdffe868
(Assignee)

Comment 14

4 years ago
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.
Attachment #8394550 - Flags: review?(jwalden+bmo) → review+
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?  :-)
Attachment #8394566 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 16

4 years ago
(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.
(Assignee)

Comment 17

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a673af0b3f22
https://hg.mozilla.org/integration/mozilla-inbound/rev/012fea76225f
Flags: in-testsuite-
Target Milestone: --- → mozilla31
https://hg.mozilla.org/mozilla-central/rev/a673af0b3f22
https://hg.mozilla.org/mozilla-central/rev/012fea76225f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.