Comparison between signed and unsigned integer expressions in nsAccessible.h

RESOLVED FIXED in mozilla7

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jdm, Assigned: tbsaunde)

Tracking

(Blocks: 1 bug)

Trunk
mozilla7
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
../../../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
(Reporter)

Comment 1

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

Comment 2

6 years ago
Created attachment 536184 [details] [diff] [review]
patch
Attachment #536184 - Flags: review?(surkov.alexander)
(Assignee)

Updated

6 years ago
Blocks: 389800

Comment 3

6 years ago
Should we also change
   PRUint32 mFlags;
to
   PRInt32 mFlags;
?
(Assignee)

Comment 4

6 years ago
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.
Attachment #536184 - Flags: review?(surkov.alexander)

Comment 5

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

What exactly makes a warning?
(Assignee)

Comment 6

6 years ago
(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

6 years ago
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.
(Assignee)

Comment 8

6 years ago
(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

6 years ago
(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.
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
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

6 years ago
(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?
(Assignee)

Comment 13

6 years ago
(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

6 years ago
(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.
(Assignee)

Comment 15

6 years ago
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.
Attachment #536184 - Attachment is obsolete: true
Attachment #536407 - Flags: review?(surkov.alexander)

Comment 16

6 years ago
Comment on attachment 536407 [details] [diff] [review]
patch2

use static_cast over implicit casting
Attachment #536407 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 17

6 years ago
Created attachment 536678 [details] [diff] [review]
updated patch

use static_cast<ChildrenFlags>
Attachment #536407 - Attachment is obsolete: true
Comment on attachment 536678 [details] [diff] [review]
updated patch

Looks right. C++ style casting always preferred.
Attachment #536678 - Flags: review+
Attachment #536678 - Flags: review?(surkov.alexander)
Attachment #536678 - Flags: review?(surkov.alexander)
(Assignee)

Comment 19

6 years ago
landed http://hg.mozilla.org/mozilla-central/rev/fd74c6c7e102
(Assignee)

Comment 20

6 years ago
landed http://hg.mozilla.org/mozilla-central/rev/fd74c6c7e102
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
(Assignee)

Comment 21

6 years ago
WELL, THIS SHOULD BE FIXED, BUT BUG 661392 =[
Resolution: INVALID → FIXED
(Assignee)

Updated

6 years ago
Assignee: nobody → trev.saunders
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.