660693 - Comparison between signed and unsigned integer expressions in nsAccessible.h

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Josh Matthews [:jdm]]

../../../dist/include/nsAccessible.h: In member function ‘bool nsAccessible::IsChildrenFlag(nsAccessible::ChildrenFlags) const’:
../../../dist/include/nsAccessible.h:521: warning: comparison between signed and unsigned integer expressions

Comment 1

[Josh Matthews [:jdm]]

520   inline bool IsChildrenFlag(ChildrenFlags aFlag) const
521     { return (mFlags & kChildrenFlagsMask) == aFlag; }

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 3

[Ginn Chen]

Should we also change
   PRUint32 mFlags;
to
   PRInt32 mFlags;
?

Comment 4

[Trevor Saunders (:tbsaunde)]

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

I'm not sure we need to do that, but I don't see a reason not to make it a PRInt32 and that seems safer so I'll do that.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> Comment on attachment 536184 [details] [diff] [review] [review]
> patch

What exactly makes a warning?

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 536184 [details] [diff] [review] [review] [review]
> > patch
> 
> What exactly makes a warning?

I think gcc is representing variables of the enum type as signed integers, which although my check was brief doesn't seem to be contrairy to the spec.

Comment 7

[alexander :surkov (:asurkov)]

we shouldn't rely on enum as signed or unsigned int, we should deal with it as with type (in our case ChildrenFlags), so if we get failures (because there's no autocasting) then we should do explicit casting I think.

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to comment #7)
> we shouldn't rely on enum as signed or unsigned int, we should deal with it
> as with type (in our case ChildrenFlags), so if we get failures (because
> there's no autocasting) then we should do explicit casting I think.

I believe the spec says that a signed integer should always work, and unsidgned is fine but the implementation may warn about it, using  signed int as the type of kChildrenFlags and mFlags is always fine.  However I don't think there's any technical issue with that approach if you want to use it.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)

> using  signed
> int as the type of kChildrenFlags and mFlags is always fine.

it's not good idea to use signed int for something that can't ever be signed.

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > using  signed
> > int as the type of kChildrenFlags and mFlags is always fine.
> 
> it's not good idea to use signed int for something that can't ever be signed.

sure,   ideally we'd only use a 2 bit wide integer here, but that doesn't exist.

Comment 11

[Trevor Saunders (:tbsaunde)]

On the other hand enum foo { eFoo1 = 0, eFoo2 = 1, eFoo3 = 2}; is a little odd, and I'd be tempted to use const int foo1 = 0 ... here instead.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> On the other hand enum foo { eFoo1 = 0, eFoo2 = 1, eFoo3 = 2}; is a little
> odd, and I'd be tempted to use const int foo1 = 0 ... here instead.

what's odd having a type for that?

Comment 13

[Trevor Saunders (:tbsaunde)]

(In reply to comment #12)
> (In reply to comment #11)
> > On the other hand enum foo { eFoo1 = 0, eFoo2 = 1, eFoo3 = 2}; is a little
> > odd, and I'd be tempted to use const int foo1 = 0 ... here instead.
> 
> what's odd having a type for that?

I'm not really sure, I think it mostly has to do with the meaning of the word enumerate.  To Me  a enum type is a type where a value will have one of a set. of values at a time, and not b the combination of several values.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> > what's odd having a type for that?
> 
> I'm not really sure, I think it mostly has to do with the meaning of the
> word enumerate.  To Me  a enum type is a type where a value will have one of
> a set. of values at a time, and not b the combination of several values.

correct, that's what happens for ChildrenFlags, mFlags is never used to keep several flags from ChildrenFlags.

Comment 15

[Trevor Saunders (:tbsaunde)]

Attachment: patch2

once we've anded the non children flags bits off of mFlags we should have a valid ChildrenFlags enum so lets cast to that.

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 536407 [details] [diff] [review]
patch2

use static_cast over implicit casting

Comment 17

[Trevor Saunders (:tbsaunde)]

Attachment: updated patch

use static_cast<ChildrenFlags>

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 536678 [details] [diff] [review]
updated patch

Looks right. C++ style casting always preferred.

Comment 19

[Trevor Saunders (:tbsaunde)]

landed http://hg.mozilla.org/mozilla-central/rev/fd74c6c7e102

Comment 20

[Trevor Saunders (:tbsaunde)]

landed http://hg.mozilla.org/mozilla-central/rev/fd74c6c7e102

Comment 21

[Trevor Saunders (:tbsaunde)]

WELL, THIS SHOULD BE FIXED, BUT BUG 661392 =[