Closed Bug 919865 Opened 8 years ago Closed 8 years ago

Replace #ifdef DEBUG with DebugOnly<> in a few spots in layout/generic

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: dholbert, Assigned: dholbert)

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch fix v1 (obsolete) — Splinter Review
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #808983 - Flags: review?(matspal)
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 808983 [details] [diff] [review]
fix v1

In the first hunk, the "aNextInFlow->GetPrevInFlow()" adds a virtual
call in Opt builds.  Perhaps it's possible to do the assignment inside
the assertion?

It looks like most of the other ones could fit on the same line?
Attachment #808983 - Flags: review?(matspal) → review-
(In reply to Mats Palmgren (:mats) from comment #2)
> In the first hunk, the "aNextInFlow->GetPrevInFlow()" adds a virtual
> call in Opt builds.  Perhaps it's possible to do the assignment inside
> the assertion?

Oops! thanks for catching that.  I just won't bother with that hunk -- the variable is used later in the function, too, and moving the assignment into the assertion feels hackier than what we have right now, so I'll just leave that as-is.

> It looks like most of the other ones could fit on the same line?

True. I'll fix those and re-post.
Attached patch [ignore] (obsolete) — Splinter Review
This version has that first hunk removed, and the remaining hunks converted to put things on one line.

(In the final hunk, I reduced the odd 4-space indentation in the contextual code, since most of the rest of that file uses our normal 2-space indentation, and since I needed to reduce it anyway to get it into 80 characters.)
Attachment #808983 - Attachment is obsolete: true
Attachment #809557 - Flags: review?(matspal)
Comment on attachment 809557 [details] [diff] [review]
[ignore]

sorry, forgot to qref; let's try that again
Attachment #809557 - Attachment description: fix v2 → [ignore]
Attached patch fix v2Splinter Review
ok, here's the right version.
Attachment #809557 - Attachment is obsolete: true
Attachment #809557 - Flags: review?(matspal)
Attachment #809558 - Flags: review?(matspal)
Comment on attachment 809558 [details] [diff] [review]
fix v2

> moving the assignment into the assertion feels hackier than what we have
> right now, so I'll just leave that as-is.

Yeah, that's a good call.  r=mats
Attachment #809558 - Flags: review?(matspal) → review+
needinfo=me to remember to land this
Flags: needinfo?(dholbert)
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/d43432aad57d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.