Closed
Bug 665686
Opened 10 years ago
Closed 10 years ago
Fix unused variable warnings in layout/style/
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: emorley, Assigned: emorley)
References
(Blocks 1 open bug)
Details
(Whiteboard: [build_warning])
Attachments
(1 file, 1 obsolete file)
3.59 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
layout/style/nsCSSDataBlock.cpp:302:23: warning: unused variable 'iProp' http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSDataBlock.cpp#302 layout/style/nsCSSDataBlock.cpp:393:27: warning: unused variable 'iProp' http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSDataBlock.cpp#393 layout/style/nsRuleNode.cpp:705:10: warning: unused variable 'cY' http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#705
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #540585 -
Flags: review?(dbaron)
Comment 2•10 years ago
|
||
Comment on attachment 540585 [details] [diff] [review] Fix build warnings >+#include "mozilla/Util.h" // for DebugOnly Don't comment the reason for the include, as it could end up being used for other things. >- nsCSSProperty iProp = PropertyAtCursor(cursor); >+ mozilla::DebugOnly<nsCSSProperty> iProp = PropertyAtCursor(cursor); > NS_ABORT_IF_FALSE(!nsCSSProps::IsShorthand(iProp), "out of range"); This is too fancy. Just do: NS_ABORT_IF_FALSE(!nsCSSProps::IsShorthand(PropertyAtCursor(cursor)), "out of range"); >@@ -385,17 +386,17 @@ nsCSSExpandedDataBlock::ComputeSize() > { > ComputeSizeResult result = {0, 0}; > for (size_t iHigh = 0; iHigh < nsCSSPropertySet::kChunkCount; ++iHigh) { > if (!mPropertiesSet.HasPropertyInChunk(iHigh)) > continue; > for (size_t iLow = 0; iLow < nsCSSPropertySet::kBitsInChunk; ++iLow) { > if (!mPropertiesSet.HasPropertyAt(iHigh, iLow)) > continue; >- nsCSSProperty iProp = nsCSSPropertySet::CSSPropertyAt(iHigh, iLow); >+ mozilla::DebugOnly<nsCSSProperty> iProp = nsCSSPropertySet::CSSPropertyAt(iHigh, iLow); > NS_ABORT_IF_FALSE(!nsCSSProps::IsShorthand(iProp), "out of range"); > NS_ABORT_IF_FALSE(PropertyAt(iProp)->GetUnit() != eCSSUnit_Null, > "null value while computing size"); This is not an appropriate use of DebugOnly, since it's intended for cases where you *do* want to call the function but *don't* want the result for non-DEBUG. Here we don't need to call the function (though it's simple). Just use #ifdef DEBUG. >diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp >+#include "mozilla/Util.h" // for DebugOnly Again, no comment. >- PRBool cY = SetCoord(valY, aCoordY, aParentY, aMask, aStyleContext, >+ mozilla::DebugOnly<PRBool> cY = SetCoord(valY, aCoordY, aParentY, aMask, aStyleContext, > aPresContext, aCanStoreInRuleTree); > NS_ABORT_IF_FALSE(cX == cY, "changed one but not the other"); This is the intended use of DebugOnly.
Attachment #540585 -
Flags: review?(dbaron) → review-
Comment 3•10 years ago
|
||
(In reply to comment #2) > >- PRBool cY = SetCoord(valY, aCoordY, aParentY, aMask, aStyleContext, > >+ mozilla::DebugOnly<PRBool> cY = SetCoord(valY, aCoordY, aParentY, aMask, aStyleContext, > > aPresContext, aCanStoreInRuleTree); > > NS_ABORT_IF_FALSE(cX == cY, "changed one but not the other"); > > This is the intended use of DebugOnly. ... but you should rewrap the line to wrap in less than 80 characters (i.e., move aStyleContext to the next line).
Assignee | ||
Comment 4•10 years ago
|
||
Updated for comment 2 and comment 3 feedback. > >+ mozilla::DebugOnly<nsCSSProperty> iProp = nsCSSPropertySet::CSSPropertyAt(iHigh, iLow); > > This is not an appropriate use of DebugOnly, since it's intended for cases > where you *do* want to call the function but *don't* want the result for > non-DEBUG. Here we don't need to call the function (though it's simple). Thanks for spotting that, the 'set' in nsCSSPropertySet made me think it wasn't a no-op function and so would still be required even without the NS_ABORT_IF_FALSE. (I'm not yet really very familiar with layout code).
Attachment #540585 -
Attachment is obsolete: true
Attachment #540699 -
Flags: review?(dbaron)
Assignee | ||
Updated•10 years ago
|
Attachment #540699 -
Attachment description: Fix build warnings → Fix build warnings v2
Comment 5•10 years ago
|
||
Comment on attachment 540699 [details] [diff] [review] Fix build warnings v2 r=dbaron. Thanks for fixing this.
Attachment #540699 -
Flags: review?(dbaron) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/858a29a7f052
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in
before you can comment on or make changes to this bug.
Description
•