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)

(Assignee)

Description

6 years ago
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

6 years ago
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).
(Assignee)

Comment 4

6 years ago
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)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Updated

6 years ago
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.