Closed Bug 795238 Opened 8 years ago Closed 8 years ago

Ensure nsresult is unsigned

Categories

(Core :: XPCOM, defect)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: neil, Assigned: ehsan)

References

Details

Attachments

(1 file)

After the main nsresult enum landing, there were several places where MSVC was interpreting nsresult as signed even though its range exceeds INT_MAX. We should use MSVC's enum base type extension to force MSVC to treat it as unsigned. This should also allow us to back out the MSVC-specific workarounds.
That extension actually became part of the C++11 standard.  The right way to check for this is #if defined(MOZ_HAVE_CXX11_ENUM_TYPE), which currently will be true for _MSC_VER >= 1400 (see mfbt/Attributes.h).  FWIW, the C++ standard already guarantees that the underlying type in our case must be at least the size of uint32_t, so this is a bug in MSVC per spec (as well as per common sense!).

It looks like bug 794734 already added this workaround for MSVC 64-bit.  IMO, the line

  #if defined(__cplusplus) && defined(_MSC_VER) && defined(_M_X64)

should just be changed to

  #if defined(__cplusplus) && defined(MOZ_HAVE_CXX11_ENUM_TYPE)

and that should be fine.
Attached patch Patch (v1)Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #666006 - Flags: review?(ayg)
So, do I need to file a separate bug on backing out the previous MSVC-specific workarounds?
(In reply to neil@parkwaycc.co.uk from comment #3)
> So, do I need to file a separate bug on backing out the previous
> MSVC-specific workarounds?

Sure.
Comment on attachment 666006 [details] [diff] [review]
Patch (v1)

LGTM.  Sorry for the long delay -- I should have changed my Bugzilla name to note I was away for a week or so.
Attachment #666006 - Flags: review?(ayg) → review+
Blocks: 799432
https://hg.mozilla.org/integration/mozilla-inbound/rev/13d96461b2da
Target Milestone: --- → mozilla19
Pushed a follow-up:

https://hg.mozilla.org/integration/mozilla-inbound/rev/52c773840b00

We need mozilla/Attributes.h #included there since that's where MOZ_HAVE_CXX11_ENUM_TYPE is defined.
https://hg.mozilla.org/mozilla-central/rev/13d96461b2da
https://hg.mozilla.org/mozilla-central/rev/52c773840b00
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.