Closed
Bug 1224951
Opened 9 years ago
Closed 9 years ago
Fix -Wunreachable-code warnings in layout
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: cpeterson, Assigned: cpeterson)
References
Details
Attachments
(2 files)
12.09 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.10 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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+
Comment 3•9 years ago
|
||
(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).
Assignee | ||
Comment 5•9 years ago
|
||
(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.
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/be237dc0a0b5 https://hg.mozilla.org/integration/mozilla-inbound/rev/597c3c78da4a
Comment 11•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/be237dc0a0b5 https://hg.mozilla.org/mozilla-central/rev/597c3c78da4a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Assignee | ||
Updated•8 years ago
|
Blocks: Wunreachable-code
You need to log in
before you can comment on or make changes to this bug.
Description
•