Build warning caused by bug in xpidl, converting unsigned positive number to negative number

VERIFIED FIXED

Status

()

Core
XPCOM
P3
normal
VERIFIED FIXED
19 years ago
10 years ago

People

(Reporter: Daniel Bratell, Assigned: Mike McCabe)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

19 years ago
During a build I get lots of
..\..\..\..\dist\include\nsIFileSpecWithUI.h(45) : warning C4146: unary minus
operator applied to unsigned type, result still unsigned

Looking at the code this is caused by
    enum { eExtraFilter = -2147483648 };

which is generated by xpidl when compiling nsIFileSpecWithUI.idl. The source
line in that file is

const unsigned long eExtraFilter = 1<<31;

Apparently a "unsigned long 1<<31" is converted to "-2147483648" which surely is
wrong. I don't know if this causes a bug when using the file. If I get the bit
arithmetic right "-2147483648" and "2147483648" is the same bit pattern when
using two complement, but we might not always be so lucky.

I didn't know what component to assign this bug to so be free to reassign as
appropriate.
(Reporter)

Comment 1

19 years ago
Found the xpidl component in Bugzilla. Reassigning to mccabe.
Assignee: leger → mccabe
Component: Browser-General → xpidl
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
QA Contact: leger → rginda
(Assignee)

Comment 2

19 years ago
Thanks for finding this.

Marking as assigned and changing QA contact.
(Assignee)

Comment 3

19 years ago
Created attachment 3307 [details] [diff] [review]
diff to xpidl_header.c to respect sign and long-ness when generating consts
(Assignee)

Comment 4

19 years ago
Attaching proposed fix.  With m12 and dogfood, it might be a while before I land
it.

bratell -

I'm not totally familiar with just what can go into an enum (signed, unsigned,
long, etc.) or with number type conversions and the printf % formats I used in
the fix.  It seems to work...

Could you take a look and tell me if what I'm doing makes sense?  Am I doing a
lot of extra work to do something that could be handled by automatic conversions
except in the case you noticed?

Thanks -

Mike
(Reporter)

Comment 5

19 years ago
I'm no guru either but I've applied the patch and the warnings are gone. I'm a
little unsure about the "ld" and "lu" arguments to printf. I _think_ that 'l'
specifies that the given variable is a long variable but I couldn't find the
type of IDL_INTEGER(dcl->const_exp).value so I don't know what's correct. As
both int and long are 32-bit on all modern platforms I'm aware of, it may be a
benign big in the worst case. Apart from that the patch looks good to me.
(Assignee)

Comment 6

19 years ago
*** Bug 22014 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

18 years ago
Fix checked in.  Thanks for confirming it :)
(Assignee)

Updated

18 years ago
Status: RESOLVED → REOPENED
(Assignee)

Comment 8

18 years ago
Reopening bug.

I had to revert this fix, because it caused mac, solaris and linux/ppc to
generate '0' for consts.  Needless to say, this lead to trouble elsewhere in the
tree.  I'm looking for clues as to why this triggered a cross-platform
difference... changed file was xpidl_header.c.

Updated

18 years ago
Resolution: FIXED → ---

Comment 9

18 years ago
Clearing FIXED resolution due to reopen.
(Assignee)

Updated

18 years ago
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

18 years ago
Fix checked in.

Comment 11

18 years ago
Yes. Old bug.  fixed on all platforms for some time.  Marking Verified.
Status: RESOLVED → VERIFIED

Updated

10 years ago
Component: xpidl → XPCOM
QA Contact: rginda → xpcom
You need to log in before you can comment on or make changes to this bug.