588718 - warning: comparison between signed and unsigned integer expressions in nsSMILTimeValueSpec::CheckEventDetail

Status: RESOLVED DUPLICATE of bug 587910

(Core :: SVG, defect)

Description

[timeless]

content/smil/nsSMILTimeValueSpec.cpp:
 In member function ‘PRBool nsSMILTimeValueSpec::CheckEventDetail(nsIDOMEvent*)’:
427: warning: comparison between signed and unsigned integer expressions

Comment 1

[timeless]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 467356 [details] [diff] [review]
patch

r=dholbert, but can you add just before this return statement:
  NS_ABORT_IF_FALSE(detail >= 0, "received negative repeat count?");

(birtles, just as a sanity-check, can you confirm that the above NS_ABORT_IF_FALSE is correct -- i.e. that I'm not misinterpreting what's going on in that function?)

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> r=dholbert, but can you add just before this return statement:
Er, I meant to say "can you add this before the return statement"
(sorry, it's late :))

Comment 4

[Brian Birtles (:birtles)]

(In reply to comment #2)
> Comment on attachment 467356 [details] [diff] [review]
> patch
> 
> r=dholbert, but can you add just before this return statement:
>   NS_ABORT_IF_FALSE(detail >= 0, "received negative repeat count?");
> 
> (birtles, just as a sanity-check, can you confirm that the above
> NS_ABORT_IF_FALSE is correct -- i.e. that I'm not misinterpreting what's going
> on in that function?)

Actually, the detail could potentially be negative since we accept synthesised events. I don't think we want to abort in the case where we get something wacky from content right?

So maybe this should be:
  return detail > 0 && (PRUint32)detail == mParams.mRepeatIterationOrAccessKey;

In the case where detail == 0 (which will only happen for synthesised events) we want to return false.

Sorry, for not picking this up before, I'm building on Windows (where there's no warning for this case) and haven't got in the habit of going through TryServer logs to pick this sort of thing up.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
Ah, I'd forgotten about synthesized events -- thanks Brian!

> So maybe this should be:
>   return detail > 0 && (PRUint32)detail == mParams.mRepeatIterationOrAccessKey;

That sounds good to me.  Timeless, mind tweaking the patch to that? (just adding "detail > 0 &&" to your existing patch)

Comment 6

[timeless]

Attachment: only positive details