Reintroduce long long support in CheckedInt

RESOLVED FIXED in mozilla23

Status

()

Core
MFBT
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Trunk
mozilla23
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 746663 [details] [diff] [review]
long long support

So, I can't land bug 869194 until this is done, because in OggCodecState.cpp we try to construct CheckedInt's from ogg_int64_t, and that ogg_int64_t type comes straight out of libogg where it is a typedef for long long. As I don't suppose that we want to depart from upstream libogg or fight a war about their choice of integer types, the path of least resistance seems to be to re-add long long in CheckedInt.
Attachment #746663 - Flags: review?(jwalden+bmo)

Comment 1

5 years ago
Comment on attachment 746663 [details] [diff] [review]
long long support

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

Hmm.  So int64_t works now because we explicitly add |int64_t| support.  Where int64_t is long int, long long support isn't added.  Where int64_t is long long, long long support gets incidentally picked up.  Right?  (And mutatis mutandis for uint64_t and unsigned long long.)

Remind me why long long isn't supported?  The best I could find, in the original bug moving CheckedInt into mfbt, was *maybe* that a request to not have a has-long-long check got understood as a request to remove long-long support entirely, instead of as just making long-long support always be present.
Attachment #746663 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 2

5 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> Comment on attachment 746663 [details] [diff] [review]
> long long support
> 
> Review of attachment 746663 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm.  So int64_t works now because we explicitly add |int64_t| support. 
> Where int64_t is long int, long long support isn't added.  Where int64_t is
> long long, long long support gets incidentally picked up.  Right?  (And
> mutatis mutandis for uint64_t and unsigned long long.)

Right. int64_t is explicitly supported, long long is not, so long long is only accidentally supported if it happens to be /the same/ type as int64_t.

> Remind me why long long isn't supported?  The best I could find, in the
> original bug moving CheckedInt into mfbt, was *maybe* that a request to not
> have a has-long-long check got understood as a request to remove long-long
> support entirely, instead of as just making long-long support always be
> present.

Ah, I can see how we could have had such a misunderstanding, although I don't remember.

As I see it, long long is that weird thing that you have to check support for, and can give you compiler warnings, so is best avoided. It was initially supported because the initial version of CheckedInt worked on short/int/long types and in that scheme, the only way to make sure that you cover 64bit integers is to support long long. When CheckedInt gained support for stdint explicit-sized types as well in a second IsSupported pass, the need for long long seemed to have vanished. The only reason why I think we need to re-introduce it now is that the libogg example shows that sometimes we have to deal with types defined in headers that we don't control and probably don't want to locally patch.

Should the long long support stuff be enclosed in a HAVE_LONG_LONG kind of preprocessor check?
(Assignee)

Comment 3

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/9a8c7cd83f22
Assignee: nobody → bjacob
Target Milestone: --- → mozilla23

Comment 4

5 years ago
(In reply to Benoit Jacob [:bjacob] from comment #2)
> Should the long long support stuff be enclosed in a HAVE_LONG_LONG kind of
> preprocessor check?

Everything supports long long, without warnings as best as I know.  I don't think we need a check.  If it happens that it introduces warnings somewhere, we can loop back then and do something.  (Likely just __extension__ it or something, behind the right macros and such.)
https://hg.mozilla.org/mozilla-central/rev/9a8c7cd83f22
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.