Fix unused variable warnings in layout/style/

RESOLVED FIXED in mozilla7

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: emorley, Assigned: emorley)

Tracking

(Blocks: 1 bug)

Trunk
mozilla7
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning])

Attachments

(1 attachment, 1 obsolete attachment)

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
Attachment #540585 - Flags: review?(dbaron)
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-
(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).
Attachment #540585 - Attachment is obsolete: true
Attachment #540699 - Flags: review?(dbaron)
Attachment #540699 - Attachment description: Fix build warnings → Fix build warnings v2
Comment on attachment 540699 [details] [diff] [review]
Fix build warnings v2

r=dbaron.  Thanks for fixing this.
Attachment #540699 - Flags: review?(dbaron) → review+
Keywords: checkin-needed
Landed on inbound.
Keywords: checkin-needed
Pushed:
http://hg.mozilla.org/mozilla-central/rev/858a29a7f052
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.