Closed Bug 1049758 Opened 5 years ago Closed 5 years ago

fixup copy and move ctors for children iterators

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Details

Attachments

(1 file)

No description provided.
Comment on attachment 8468621 [details] [diff] [review]
fixup copy and move ctors for children iterators

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

nit: The local style in this file is for the members in the initialization list to line up with the other members, not with the colon.

r+ ignoring AllChildrenIterator because it's not in the tree.

::: content/base/src/ChildIterator.h
@@ +131,5 @@
>  
> +  explicit FlattenedChildIterator(FlattenedChildIterator&& aOther)
> +    : ExplicitChildIterator(Move(aOther)), mXBLInvolved(aOther.mXBLInvolved) {}
> +
> +  FlattenedChildIterator(const FlattenedChildIterator& aOther)

According to the style guide, it's not necessary to mark copy and move constructors as explicit. I'm not sure if we gain anything here, but if you are going to do it, this constructor should also be marked explicit (unless we can't for some other reason). I would prefer if none of these constructors were marked as explicit.

@@ +168,5 @@
>      FlattenedChildIterator(aNode, (aFlags & nsIContent::eAllButXBL)),
>      mOriginalContent(aNode), mFlags(aFlags),
>      mPhase(eNeedBeforeKid) {}
>  
> +  explicit AllChildrenIterator(AllChildrenIterator&& aOther)

This looks new.
Attachment #8468621 - Flags: review?(wchen) → review+
> r+ ignoring AllChildrenIterator because it's not in the tree.

it's in the tree now I landed the patch adding it a couple days ago.

> ::: content/base/src/ChildIterator.h
> @@ +131,5 @@
> >  
> > +  explicit FlattenedChildIterator(FlattenedChildIterator&& aOther)
> > +    : ExplicitChildIterator(Move(aOther)), mXBLInvolved(aOther.mXBLInvolved) {}
> > +
> > +  FlattenedChildIterator(const FlattenedChildIterator& aOther)
> 
> According to the style guide, it's not necessary to mark copy and move
> constructors as explicit. I'm not sure if we gain anything here, but if you
> are going to do it, this constructor should also be marked explicit (unless
> we can't for some other reason). I would prefer if none of these
> constructors were marked as explicit.

I'll just remove all the explicits, I think it may be useful, but I forget too.

> @@ +168,5 @@
> >      FlattenedChildIterator(aNode, (aFlags & nsIContent::eAllButXBL)),
> >      mOriginalContent(aNode), mFlags(aFlags),
> >      mPhase(eNeedBeforeKid) {}
> >  
> > +  explicit AllChildrenIterator(AllChildrenIterator&& aOther)
> 
> This looks new.

true, does it seem ok?
Yeah, it seems OK.
Looks like the build bustage was on all non-DEBUG builds:

../../dist/include/mozilla/dom/ChildIterator.h:179:30: error: class 'mozilla::dom::AllChildrenIterator' does not have any field named 'mMutationGuard'
../../dist/include/mozilla/dom/ChildIterator.h:179:52: error: 'class mozilla::dom::AllChildrenIterator' has no member named 'mMutationGuard'
https://hg.mozilla.org/mozilla-central/rev/d046865456ff
Assignee: nobody → trev.saunders
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: needinfo?(trev.saunders)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.