Last Comment Bug 768570 - Fix all the warnings in CheckedInt
: Fix all the warnings in CheckedInt
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on:
Blocks: 828741 756397
  Show dependency treegraph
 
Reported: 2012-06-26 11:25 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2013-01-09 16:36 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix CheckedInt warnings (3.25 KB, patch)
2012-06-26 11:25 PDT, Benoit Jacob [:bjacob] (mostly away)
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-06-26 11:25:07 PDT
Created attachment 636801 [details] [diff] [review]
fix CheckedInt warnings

Attached patch fixes all the warnings reported by g++-4.6 -Wall -Wextra -pedantic. I have actually seen those while building Firefox, so it matters.

There are 2 fixes:

- In CheckedInt.h, DoesRangeContainRange helper introduce to avoid warnings about >= and <= comparisons that are always true due to the types at hand.

- In TestCheckedInt.cpp, we had a warning about integer overflow at compile time. 'Fixed' it by replacing a literal 1 by one.value().

Mo: you'll probably want this as well in WebKit.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-28 18:17:07 PDT
Comment on attachment 636801 [details] [diff] [review]
fix CheckedInt warnings

Review of attachment 636801 [details] [diff] [review]:
-----------------------------------------------------------------

You don't have to sell me on fixing the in-range warning.  We've seen this before in the JS engine, actually; I wonder if this might even plausibly be worth generalizing at all, so people can do templaty range tests a particular way and never have to worry about in-rangeness.  Probably not.  (The JS engine cases involved integers and doubles, too, and your tricks don't generalize to floating point types anyway.)
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-06-28 19:30:05 PDT
Yeah, it would have to be written differently to accomodate floating point types. If I had to write it with such generality in mind, here is how I would proceed. I would write template helpers that return, for a given type, an integer telling how big the max value is (and how big negative the min value is). Then the range comparison would boil down to a >= comparison on these integers. For example you could return this for the max value:

   int8 ->   1
   uint8 ->  2
   int16 ->  3
   uint16 -> 4
   int32 ->  5
   uint32 -> 6
   int64 ->  7
   uint64 -> 8
   float ->  9
   double -> 10

Of course if by 'range' you rather mean the range of exactly representable integers, then the ordering is different:

   int8 ->   1
   uint8 ->  2
   int16 ->  3
   uint16 -> 4
   float ->  5
   int32 ->  6
   uint32 -> 7
   double -> 8
   int64 ->  9
   uint64 -> 10

And ditto for the min values (this time you have repeated values, should return 0 for all unsigned types).
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-07-05 07:19:33 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/2b9bea1ee9e9
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-07-05 17:20:05 PDT
https://hg.mozilla.org/mozilla-central/rev/2b9bea1ee9e9

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