Closed Bug 1224951 Opened 4 years ago Closed 4 years ago

Fix -Wunreachable-code warnings in layout

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(2 files)

Remove some unreachable break statements after return statements. Remove some unnecessary function prototypes. Remove some commented-out macro definitions that are not referenced by other code.

layout/base/nsCSSFrameConstructor.cpp:12067:12: warning: will never be executed [-Wunreachable-code]
layout/base/nsPresContext.cpp:2861:10: warning: will never be executed [-Wunreachable-code]
layout/generic/nsFrameSetFrame.cpp:730:11: warning: will never be executed [-Wunreachable-code]
layout/generic/nsFrameSetFrame.cpp:725:11: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:268:62: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:269:66: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:270:68: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:271:75: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:272:73: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:273:81: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:274:69: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:275:60: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:276:68: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:277:68: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:279:18: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:290:62: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:291:66: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:292:68: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:293:75: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:294:73: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:295:81: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:296:69: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:297:60: warning: will never be executed [-Wunreachable-code]
layout/generic/nsSelection.cpp:5657:7: warning: will never be executed [-Wunreachable-code]
layout/mathml/nsMathMLmrootFrame.cpp:405:5: warning: will never be executed [-Wunreachable-code]
Attachment #8687707 - Flags: review?(dholbert)
Fix nsPresContext::SizeOfExcludingThis() size calculation. A -Wunreachable-code warning pointed out that the second line of code was unreachable and not actually used when calculating the size!
Attachment #8687708 - Flags: review?(dholbert)
Comment on attachment 8687708 [details] [diff] [review]
fix_nsPresContext-SizeOfExcludingThis.patch

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

Yikes, good catch!

Slight nit: please include "part 1" / "part 2" in the commit messages here.

(Otherwise, from looking at this single commit in isolation, it appears (incorrectly) that this bug specifically covers this one issue (fixing nsPresContext::SizeOfExcludingThis).  But really there's a bit more going on here.)
Attachment #8687708 - Flags: review?(dholbert) → review+
(have to run at the moment; probably won't get to the larger patch (part 1) until later today)
Comment on attachment 8687708 [details] [diff] [review]
fix_nsPresContext-SizeOfExcludingThis.patch

One other nit:  Gecko style has the + at the end of the first line rather than the start of the second (thus removing the need to modify the second changed line at all).
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #4)
> One other nit:  Gecko style has the + at the end of the first line rather
> than the start of the second (thus removing the need to modify the second
> changed line at all).

The Mozilla Coding Style page on MDN says:

Overlong expressions not joined by && and || should break so the operator starts on the second line and starts in the same column as the start of the expression in the first line. This applies to ?:, binary arithmetic operators including +, and member-of operators …

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators
That's interesting; existing usage of breaking at + in layout/ .cpp and .h files disagrees with that by 281 to 9; in dom by 351 to 36, and in js by 248 to 40.

It's a perfectly reasonable style; it's just inconsistent with actual usage in Gecko.
I'll defer to (and agree with) dbaron & assert that it's better to match the vast-majority of gecko code here and add the "+" on the first line.

(I slightly wonder if that coding style point was written from the perspective of front-end code? Not sure if it conforms to that guideline more than Gecko C++ code does.  If you like, it might be worth starting a thread on dev.platform RE bringing Gecko & this coding-style-guide point into alignment, one way or another.)
Comment on attachment 8687707 [details] [diff] [review]
fix_Wunreachable-code_warnings.patch

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

r=me with nits below addressed:

Commit message:
> Bug NUMBER - Fix -Wunreachable-code warnings in layout. r?

"Bug 1224951 part 1", and maybe s/warnings/clang warnings/.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +12061,5 @@
>      if (aItems.AreAllItemsBlock()) {
>        return false;
>      }
>  
>      // We might have some inline kids for this block.  Just reconstruct.

Nit: So, every "Just reconstruct"-type comment in this code is followed by (and clearly documenting) a "break" statement.  But this patch is removing the last "break" statement here, which leaves this comment kind of awkwardly floating without any code that it's clearly referring to.

To address this, please reword the comment to end with something like "Just drop out of the loop & reconstruct", to make it a bit clearer what this comment is talking about, and to better tie it to the "while(0)" statement that it's now adjacent to.

::: layout/generic/nsSelection.cpp
@@ -91,5 @@
> -
> -static nsIAtom *GetTag(nsINode *aNode);
> -// returns the parent
> -static nsINode* ParentOffset(nsINode *aNode, int32_t *aChildOffset);
> -static nsINode* GetCellParent(nsINode *aDomNode);

These static-function forward-decl removals seem fine, I suppose, but I'm not sure it makes sense to include them in this patch.   I'd tend to think we should put these removals (and the corresponding "static" tweaks to their function-definitions) in a separate patch, since (unlike the rest of this patch) these removals aren't about dead/unreachable code.

(Though, since you've included them here, I'm wondering if maybe clang does flag these lines as Wunreachable for some reason? Even if it does, they seem to be qualitatively different form of "unreachable" than the rest of this patch... more like "unnecessary-forward-declaration" than "unreachable".)
Attachment #8687707 - Flags: review?(dholbert) → review+
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #4)
> One other nit:  Gecko style has the + at the end of the first line rather
> than the start of the second (thus removing the need to modify the second
> changed line at all).

OK. I moved the + to the end of the first line.

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Nit: So, every "Just reconstruct"-type comment in this code is followed by
> (and clearly documenting) a "break" statement.  But this patch is removing
> the last "break" statement here, which leaves this comment kind of awkwardly
> floating without any code that it's clearly referring to.
> 
> To address this, please reword the comment to end with something like "Just
> drop out of the loop & reconstruct", to make it a bit clearer what this
> comment is talking about, and to better tie it to the "while(0)" statement
> that it's now adjacent to.

OK. I'll update the comment.

> ::: layout/generic/nsSelection.cpp
> @@ -91,5 @@
> > -
> > -static nsIAtom *GetTag(nsINode *aNode);
> > -// returns the parent
> > -static nsINode* ParentOffset(nsINode *aNode, int32_t *aChildOffset);
> > -static nsINode* GetCellParent(nsINode *aDomNode);
> 
> (Though, since you've included them here, I'm wondering if maybe clang does
> flag these lines as Wunreachable for some reason? Even if it does, they seem
> to be qualitatively different form of "unreachable" than the rest of this
> patch... more like "unnecessary-forward-declaration" than "unreachable".)

clang didn't warn about these forward declarations. I just noticed they were unnecessary because I was curious why static functions defined in this file might have such a tangled interdependencies that they need to be forward declared. And they weren't. :)

I'll revert those changes from my patch.
https://hg.mozilla.org/mozilla-central/rev/be237dc0a0b5
https://hg.mozilla.org/mozilla-central/rev/597c3c78da4a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.