Closed Bug 868557 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Details

Attachments

(2 files)

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-
(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.
Attached patch Try againSplinter Review
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.