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
Created attachment 540585 [details] [diff] [review] Fix build warnings
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.
(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).
Created attachment 540699 [details] [diff] [review] Fix build warnings v2 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).
Comment on attachment 540699 [details] [diff] [review] Fix build warnings v2 r=dbaron. Thanks for fixing this.
Landed on inbound.
Pushed: http://hg.mozilla.org/mozilla-central/rev/858a29a7f052
or
