Bug 85748 – InlineBox::setHasBadParent should be debug only

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Rob Buis]

The method InlineBox::setHasBadParent is only used in debug, so don't provide this function in release.

[Rob Buis]

Created attachment 140436 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 140436 [details]
Patch

I'm not sure this helps much.  I guess all callers were already wrapped behind NDEBUG flags?

[Rob Buis]

Hi Eric,

(In reply to comment #2)
> (From update of attachment 140436 [details])
> I'm not sure this helps much.  I guess all callers were already wrapped behind NDEBUG flags?

Not claiming a huge speedup :) But why provide a no-op method in release that is not called? Yes the one caller is wrapped in NDEBUG.
Cheers,

Rob.

[Rob Buis]

(In reply to comment #3)
> Hi Eric,
> 
> (In reply to comment #2)
> > (From update of attachment 140436 [details] [details])
> > I'm not sure this helps much.  I guess all callers were already wrapped behind NDEBUG flags?
> 
> Not claiming a huge speedup :) But why provide a no-op method in release that is not called? Yes the one caller is wrapped in NDEBUG.
> Cheers,
> 
> Rob.

I think I should have said, method with empty body.

[WebKit Review Bot]

Comment on attachment 140436 [details]
Patch

Clearing flags on attachment: 140436

Committed r116253: <http://trac.webkit.org/changeset/116253>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Why make this change? The other way can be nicer since you don’t have to #ifdef every call site if it’s the only thing that would require an ifdef.

Also, if we’re not doing the NDEBUG version, then I see no point in making this inline.

[Rob Buis]

Hi Darin,
(In reply to comment #7)
> Why make this change? The other way can be nicer since you don’t have to #ifdef every call site if it’s the only thing that would require an ifdef.

Right, but at the moment there is only one call site, and it is in a NDEBUG section.

> Also, if we’re not doing the NDEBUG version, then I see no point in making this inline.

I am not even thinking about inline, the goal of my patch is to simply remove the method for release builds , since it is only useful in debug builds. Basically it is the way I would have done this code myself. I don't know if the release binary gets smaller or the compiler is clever enough to not generate code for this unused function (in release). Maybe the inline is enough of a clue for the compiler to do that, I just found my way clearer. Basically I happened to have this change in my git tree and I needed to get it out of the way :) If you feel strongly for the old approach, a revert is possible of course.
Cheers,

Rob.

[Darin Adler]

(In reply to comment #8)
> I don't know if the release binary gets smaller

I do know. It does not get smaller.

> the compiler is clever enough to not generate code for this unused function (in release)

It is.

> I just found my way clearer.

OK. We disagree on this. I like the old way.

I don’t think we should be changing things like this, especially in code that we’re not otherwise changing. I think fewer #ifdefs are better for code readability.

Functions that are empty inlines in release builds are a technique used all over the place in WebCore. So there should be no doubt that the technique works unless we see an actual problem.

I am not going to push you to change this in this case, but lets please not head in this direction in the future.

[Rob Buis]

Hi Darin,

(In reply to comment #9)
> (In reply to comment #8)
> > I don't know if the release binary gets smaller
> 
> I do know. It does not get smaller.
> 
> > the compiler is clever enough to not generate code for this unused function (in release)
> 
> It is.
> 
> > I just found my way clearer.
> 
> OK. We disagree on this. I like the old way.
> 
> I don’t think we should be changing things like this, especially in code that we’re not otherwise changing. I think fewer #ifdefs are better for code readability.
> 
> Functions that are empty inlines in release builds are a technique used all over the place in WebCore. So there should be no doubt that the technique works unless we see an actual problem.
> 
> I am not going to push you to change this in this case, but lets please not head in this direction in the future.

Ok, I did not know this, I'll indeed not do changes like that in the future.