Last Comment Bug 660693 - Comparison between signed and unsigned integer expressions in nsAccessible.h
: Comparison between signed and unsigned integer expressions in nsAccessible.h
Status: RESOLVED FIXED
[build_warning]
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla7
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2011-05-30 13:03 PDT by Josh Matthews [:jdm]
Modified: 2011-06-01 18:15 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (741 bytes, patch)
2011-05-30 15:15 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch2 (780 bytes, patch)
2011-05-31 14:07 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
updated patch (789 bytes, patch)
2011-06-01 11:32 PDT, Trevor Saunders (:tbsaunde)
dbolter: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2011-05-30 13:03:34 PDT
../../../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] 2011-05-30 13:08:03 PDT
520   inline bool IsChildrenFlag(ChildrenFlags aFlag) const
521     { return (mFlags & kChildrenFlagsMask) == aFlag; }
Comment 2 Trevor Saunders (:tbsaunde) 2011-05-30 15:15:06 PDT
Created attachment 536184 [details] [diff] [review]
patch
Comment 3 Ginn Chen 2011-05-30 19:17:30 PDT
Should we also change
   PRUint32 mFlags;
to
   PRInt32 mFlags;
?
Comment 4 Trevor Saunders (:tbsaunde) 2011-05-30 21:20:50 PDT
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 2011-05-30 22:43:45 PDT
(In reply to comment #4)
> Comment on attachment 536184 [details] [diff] [review] [review]
> patch

What exactly makes a warning?
Comment 6 Trevor Saunders (:tbsaunde) 2011-05-30 23:07:51 PDT
(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 2011-05-30 23:22:11 PDT
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) 2011-05-30 23:36:26 PDT
(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 2011-05-30 23:48:19 PDT
(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) 2011-05-31 00:18:12 PDT
(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) 2011-05-31 00:31:52 PDT
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 2011-05-31 00:35:51 PDT
(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) 2011-05-31 01:55:12 PDT
(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 2011-05-31 03:20:56 PDT
(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) 2011-05-31 14:07:16 PDT
Created attachment 536407 [details] [diff] [review]
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 2011-05-31 20:52:28 PDT
Comment on attachment 536407 [details] [diff] [review]
patch2

use static_cast over implicit casting
Comment 17 Trevor Saunders (:tbsaunde) 2011-06-01 11:32:59 PDT
Created attachment 536678 [details] [diff] [review]
updated patch

use static_cast<ChildrenFlags>
Comment 18 David Bolter [:davidb] 2011-06-01 11:40:34 PDT
Comment on attachment 536678 [details] [diff] [review]
updated patch

Looks right. C++ style casting always preferred.
Comment 19 Trevor Saunders (:tbsaunde) 2011-06-01 14:27:49 PDT
landed http://hg.mozilla.org/mozilla-central/rev/fd74c6c7e102
Comment 20 Trevor Saunders (:tbsaunde) 2011-06-01 14:32:15 PDT
landed http://hg.mozilla.org/mozilla-central/rev/fd74c6c7e102
Comment 21 Trevor Saunders (:tbsaunde) 2011-06-01 15:04:03 PDT
WELL, THIS SHOULD BE FIXED, BUT BUG 661392 =[

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