665686 - Fix unused variable warnings in layout/style/

Status: RESOLVED FIXED

Description

[Ed Morley [:emorley]]

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

Comment 1

[Ed Morley [:emorley]]

Created attachment 540585 [details] [diff] [review]
Fix build warnings

Comment 2

[David Baron :dbaron: ⌚️UTC+1]

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.

Comment 3

[David Baron :dbaron: ⌚️UTC+1]

(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).

Comment 4

[Ed Morley [:emorley]]

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 5

[David Baron :dbaron: ⌚️UTC+1]

Comment on attachment 540699 [details] [diff] [review]
Fix build warnings v2

r=dbaron.  Thanks for fixing this.

Comment 6

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

Landed on inbound.

Comment 7

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/858a29a7f052