Explicitly unalias list in nsStyleContext::AddChild(nsStyleContext* aChild)

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla23
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
Created attachment 745308 [details] [diff] [review]
Explicitly unalias list in nsStyleContext::AddChild(nsStyleContext* aChild)

Clang and GCC probably MSVC are not able to unalias list because they don't realize that mEmptyChild and mChild can't alias mNextSibling and mPrevSibling. If we explicitly dereference list we get better code, saving 3 instructions on clang.
Attachment #745308 - Flags: review?(dbaron)
Comment on attachment 745308 [details] [diff] [review]
Explicitly unalias list in nsStyleContext::AddChild(nsStyleContext* aChild)

>+  nsStyleContext **listPtr = aChild->mRuleNode->IsRoot() ? &mEmptyChild : &mChild;
>+  // Explicitly dereference listPtr so that compiler doesn't have to know that mNextSibling
>+  // etc. don't alias with what ever listPtr points at.
>+  nsStyleContext list = *listPtr;

This doesn't even look like it compiles; you shouldn't be able to assign an nsStyleContext* to an nsStyleContext, and you don't want to copy-construct an nsStyleContext ither.
Attachment #745308 - Flags: review?(dbaron) → review-
(Assignee)

Comment 2

6 years ago
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #1)
> Comment on attachment 745308 [details] [diff] [review]
> Explicitly unalias list in nsStyleContext::AddChild(nsStyleContext* aChild)
> 
> >+  nsStyleContext **listPtr = aChild->mRuleNode->IsRoot() ? &mEmptyChild : &mChild;
> >+  // Explicitly dereference listPtr so that compiler doesn't have to know that mNextSibling
> >+  // etc. don't alias with what ever listPtr points at.
> >+  nsStyleContext list = *listPtr;
> 
> This doesn't even look like it compiles; you shouldn't be able to assign an
> nsStyleContext* to an nsStyleContext, and you don't want to copy-construct
> an nsStyleContext ither.

Ooops, list should've been nsStyleContext *
Well, r=dbaron with that, though I'm hoping the "saving 3 instructions" came from the correct patch rather than the one that I don't think should compile.
(Assignee)

Comment 4

6 years ago
Created attachment 745403 [details] [diff] [review]
Try again
Attachment #745403 - Flags: review?(dbaron)
Attachment #745403 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/f847ac098b3e
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.