Last Comment Bug 665686 - Fix unused variable warnings in layout/style/
: Fix unused variable warnings in layout/style/
Status: RESOLVED FIXED
[build_warning]
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Ed Morley [:emorley]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: buildwarning
  Show dependency treegraph
 
Reported: 2011-06-20 13:49 PDT by Ed Morley [:emorley]
Modified: 2011-06-23 02:31 PDT (History)
3 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix build warnings (4.10 KB, patch)
2011-06-20 14:16 PDT, Ed Morley [:emorley]
dbaron: review-
Details | Diff | Splinter Review
Fix build warnings v2 (3.59 KB, patch)
2011-06-21 01:03 PDT, Ed Morley [:emorley]
dbaron: review+
Details | Diff | Splinter Review

Description Ed Morley [:emorley] 2011-06-20 13:49:57 PDT
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] 2011-06-20 14:16:25 PDT
Created attachment 540585 [details] [diff] [review]
Fix build warnings
Comment 2 David Baron :dbaron: ⌚️UTC-10 2011-06-20 19:02:48 PDT
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-10 2011-06-20 19:04:26 PDT
(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] 2011-06-21 01:03:29 PDT
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-10 2011-06-21 09:07:38 PDT
Comment on attachment 540699 [details] [diff] [review]
Fix build warnings v2

r=dbaron.  Thanks for fixing this.
Comment 6 :Ehsan Akhgari 2011-06-22 11:37:06 PDT
Landed on inbound.
Comment 7 Mounir Lamouri (:mounir) 2011-06-23 02:31:16 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/858a29a7f052

Note You need to log in before you can comment on or make changes to this bug.