Closed Bug 793880 Opened 7 years ago Closed 7 years ago

tautological-constant-out-of-range-compare warning in the assertion in nsCSSSelector::SetPseudoType (StyleRule.h)

Categories

(Core :: CSS Parsing and Computation, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: ehsan, Assigned: ehsan)

Details

Attachments

(1 file)

operator precedence FTW.

In file included from /Users/ehsanakhgari/moz/mozilla-central/js/xpconnect/src/nsDOMQS.h:18:
In file included from /Users/ehsanakhgari/moz/mozilla-central/content/svg/content/src/nsSVGStylableElement.h:12:
In file included from /Users/ehsanakhgari/moz/mozilla-central/content/svg/content/src/nsSVGElement.h:14:
../../dist/include/mozilla/css/StyleRule.h:186:24: warning: comparison of constant -32768 with expression of type 'nsCSSPseudoElements::Type' is always true
      [-Wtautological-constant-out-of-range-compare]
    NS_ASSERTION(aType > PR_INT16_MIN && aType < PR_INT16_MAX, "Out of bounds");
    ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../dist/include/nsDebug.h:82:11: note: expanded from macro 'NS_ASSERTION'
    if (!(expr)) {                                            \
          ^
In file included from /Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dom/bindings/AudioDestinationNodeBinding.cpp:15:
In file included from /Users/ehsanakhgari/moz/mozilla-central/js/xpconnect/src/nsDOMQS.h:18:
In file included from /Users/ehsanakhgari/moz/mozilla-central/content/svg/content/src/nsSVGStylableElement.h:12:
In file included from /Users/ehsanakhgari/moz/mozilla-central/content/svg/content/src/nsSVGElement.h:14:
../../dist/include/mozilla/css/StyleRule.h:186:48: warning: comparison of constant 32767 with expression of type 'nsCSSPseudoElements::Type' is always true
      [-Wtautological-constant-out-of-range-compare]
    NS_ASSERTION(aType > PR_INT16_MIN && aType < PR_INT16_MAX, "Out of bounds");
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../dist/include/nsDebug.h:82:11: note: expanded from macro 'NS_ASSERTION'
    if (!(expr)) {                                            \
          ^
Hrm, forget the note about operator precedence, not this bug!
Attached patch Patch (v1)Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #664247 - Flags: review?(dbaron)
Attachment #664247 - Flags: review?(dbaron) → review?(dholbert)
Comment on attachment 664247 [details] [diff] [review]
Patch (v1)

>Bug 793880 - Fix the assertion in nsCSSSelector::SetPseudoType to shut up the compiler warning; r=dbaron
>diff --git a/layout/style/StyleRule.h b/layout/style/StyleRule.h
>index 040a476..7482c82 100644
>--- a/layout/style/StyleRule.h
>+++ b/layout/style/StyleRule.h
>@@ -183,7 +183,8 @@ public:
>     return static_cast<nsCSSPseudoElements::Type>(mPseudoType);
>   }
>   void SetPseudoType(nsCSSPseudoElements::Type aType) {
>-    NS_ASSERTION(aType > PR_INT16_MIN && aType < PR_INT16_MAX, "Out of bounds");
>+    NS_ASSERTION(static_cast<int32_t>(aType) > PR_INT16_MIN &&
>+                 static_cast<int32_t>(aType) < PR_INT16_MAX, "Out of bounds");
>     mPseudoType = static_cast<int16_t>(aType);
>   }
> 

Observation: the "out of bounds" here isn't really about aType and *its* valid range -- its enum ("Type") has much more specific and tighter bounds.

Instead, the assert is an extreme-sanity-check ot be sure we're in-bounds *for mPseudoType* -- i.e. we're not about to overflow that variable.

So -- could we change the assertion message to make that clearer? ("out of bounds -- this will overflow mPseudoType")

Also: we're not expecting to hit these boundaries, but I think we technically should be checking ">=" and "<=", since those extreme values are still representable in mPseudoType.  (they're the min/max values it can take on, by definition)

So: r=me with the message clarified and the comparisons changed to >= / <=  (assuming that all makes sense to you, too)
Attachment #664247 - Flags: review?(dholbert) → review+
Yeah, both of these points make sense to me.
https://hg.mozilla.org/mozilla-central/rev/f63b7f854ce4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.