Last Comment Bug 732875 - Move CheckedInt to MFBT
: Move CheckedInt to MFBT
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
:
Mentors:
: 685775 (view as bug list)
Depends on: 685775
Blocks: 731017 740015
  Show dependency treegraph
 
Reported: 2012-03-04 20:33 PST by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-06-19 15:55 PDT (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
1. switch CheckedInt to support stdint types instead of PR int types (9.85 KB, patch)
2012-03-05 11:46 PST, Benoit Jacob [:bjacob] (mostly away)
jwalden+bmo: review+
Details | Diff | Splinter Review
2. Let CheckedInt support the 3 families of integer types: stdint, standard integer types, and PR types (42.75 KB, patch)
2012-03-05 11:49 PST, Benoit Jacob [:bjacob] (mostly away)
jwalden+bmo: review+
Details | Diff | Splinter Review
3. update CheckedInt typedefs (1.26 KB, patch)
2012-03-05 11:51 PST, Benoit Jacob [:bjacob] (mostly away)
jwalden+bmo: review+
Details | Diff | Splinter Review
4. make nonstandard features optional (4.28 KB, patch)
2012-03-05 11:53 PST, Benoit Jacob [:bjacob] (mostly away)
jwalden+bmo: review+
Details | Diff | Splinter Review
5. update CheckedInt docs (4.36 KB, patch)
2012-03-05 11:53 PST, Benoit Jacob [:bjacob] (mostly away)
jwalden+bmo: review+
Details | Diff | Splinter Review
6. switch CheckedInt to consistently using bool (8.80 KB, patch)
2012-03-05 11:55 PST, Benoit Jacob [:bjacob] (mostly away)
jwalden+bmo: review+
Details | Diff | Splinter Review
7: address Jeff's review comments (14.84 KB, patch)
2012-03-19 09:07 PDT, Benoit Jacob [:bjacob] (mostly away)
jwalden+bmo: review+
Details | Diff | Splinter Review
8: finish adapting CheckedInt to mfbt/STYLE (42.19 KB, patch)
2012-03-19 18:17 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
8: move CheckedInt to MFBT (but support for compiled tests is just a stub) (88.83 KB, patch)
2012-03-20 08:49 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
8: move CheckedInt to MFBT (with support for compiled tests, but didn't check that test failures give oranges) (90.59 KB, patch)
2012-03-20 11:03 PDT, Benoit Jacob [:bjacob] (mostly away)
jwalden+bmo: review-
Details | Diff | Splinter Review
8: move CheckedInt to MFBT, enable unit tests in mfbt/tests (92.84 KB, patch)
2012-05-01 15:55 PDT, Benoit Jacob [:bjacob] (mostly away)
jwalden+bmo: review+
Details | Diff | Splinter Review
8: (UPDATED) move CheckedInt to MFBT, enable unit tests in mfbt/tests (131.32 KB, patch)
2012-05-13 13:22 PDT, Benoit Jacob [:bjacob] (mostly away)
jacob.benoit.1: review+
Details | Diff | Splinter Review
8: (v3) move CheckedInt to MFBT, enable unit tests in mfbt/tests (132.93 KB, patch)
2012-05-14 11:18 PDT, Benoit Jacob [:bjacob] (mostly away)
jacob.benoit.1: review+
Details | Diff | Splinter Review
8: (v4) move CheckedInt to MFBT, enable unit tests in mfbt/tests (131.97 KB, patch)
2012-05-14 12:33 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
9: some further tweaks (7.11 KB, patch)
2012-05-15 10:55 PDT, Benoit Jacob [:bjacob] (mostly away)
Ms2ger: review+
Details | Diff | Splinter Review
use stdint.h types instead of PRtypes, fixes build on OpenBSD (1.36 KB, patch)
2012-05-17 03:27 PDT, Landry Breuil (:gaston)
Ms2ger: review+
Details | Diff | Splinter Review
use stdint.h types instead of PRtypes, fixes build on OpenBSD (1.39 KB, patch)
2012-05-17 05:49 PDT, Landry Breuil (:gaston)
Ms2ger: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-03-04 20:33:32 PST
Now, this isn't just about just moving files around. We have to make it MFBT-grade first. I see 2 things that need to be done:

 - 1. (trivial) replace PRInt specializations by StdInt specializations
 - 2. (nontrivial, but problem solved before) enable support for plain standard integer types as well. I already did that for the WebKit version: https://bugs.webkit.org/show_bug.cgi?id=52401  but would do it a bit more cleanly now. The problem is that it's joyously unspecified, which standard integer types may be the same type as some StdInt type. So we need to do two passes of template specialization, with the 2nd pass run from the generic fallback case from the first pass.
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2012-03-04 20:35:06 PST
Oh, and:

 - 3. Enable c++ unit tests in mfbt/ so we can move the tests over from xpcom/tests
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-04 20:47:32 PST
(In reply to Benoit Jacob [:bjacob] from comment #1)
>  - 3. Enable c++ unit tests in mfbt/ so we can move the tests over from
> xpcom/tests

\o/ I was hoping someone would do this eventually!  ;-)
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-03-05 11:46:20 PST
Created attachment 602994 [details] [diff] [review]
1. switch CheckedInt to support stdint types instead of PR int types

This is just a quick search&replace. Not very interesting in itself, but it effectively enables support for stdint types and doesn't break stuff too much so I thought it was as good a place as any to split the patch.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-03-05 11:49:43 PST
Created attachment 602996 [details] [diff] [review]
2. Let CheckedInt support the 3 families of integer types: stdint, standard integer types, and PR types

This is the only part with nontrivial code changes. The nontrivial part is explained in a comment:

 *** What's nontrivial here is that there are different families of integer types: plain integer types, stdint types,
 *** and PR types. It is merrily undefined which types from one family may be just typedefs for a type from another family.

Aside from that, this patch also rearchitects some of that code to be less ugly. Also, the integer_traits struct goes away, is replaced by split helpers doing one thing each. Plus some more trivial changes.
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-03-05 11:51:58 PST
Created attachment 602997 [details] [diff] [review]
3. update CheckedInt typedefs

I'm not too sure what to do with these typedefs. Currently CheckedInt32 is a typedef for CheckedInt<PRInt32>. It's not great to keep encouraging people to use PR int types. This patch changes these typedefs to target stdint types. I have yet to check, but this shouldn't break the mozilla build. Alternatively one could decide to remove these convenience typedefs, but the mozilla patch would be a lot larger.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-03-05 11:53:09 PST
Created attachment 602998 [details] [diff] [review]
4. make nonstandard features optional

Other non-Mozilla projects are maintaining copies of CheckedInt. This patch should make their life easier, and that would be good for us too as the diff would be smaller.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-03-05 11:53:45 PST
Created attachment 602999 [details] [diff] [review]
5. update CheckedInt docs
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-03-05 11:55:54 PST
Created attachment 603001 [details] [diff] [review]
6. switch CheckedInt to consistently using bool

This patch removes a (probably) premature optimization from CheckedInt. The mIsValid member was stored as a T, not as a bool, to reduce the number of conversions between T and bool when evaluating nested arithmetic expressions. I never did any experiment to measure the benefit of this, and I guess it's low if positive at all. Bool at least lets the compiler know that it's only 0 or 1, and makes the code easier to read for humans.
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-03-05 11:57:35 PST
The above patches are not yet moving CheckedInt to mfbt/. They are just trying to make it "mfbt-grade". Let me know if anything else needs to be done.

The actual move to mfbt should be done on top of joe's patch in bug 685775.
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-03-05 13:57:38 PST
We don't use PR*-types in mfbt, so these specializations need to live elsewhere, if they are necessary at all.
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-07 17:09:21 PST
Comment on attachment 602994 [details] [diff] [review]
1. switch CheckedInt to support stdint types instead of PR int types

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

::: xpcom/ds/CheckedInt.h
@@ +43,1 @@
>  #include "prtypes.h"

Aren't you missing an #include "mozilla/StandardInteger.h"?  I'd guess this only happens to work because of #include bootlegging.

@@ +47,5 @@
>  namespace mozilla {
>  
>  namespace CheckedInt_internal {
>  
> +/* historically, we didn't want to use std::numeric_limits here because PRInt... types may not support it,

"Historically" (caps)

@@ +356,5 @@
>    * is the type of the checked integer.
>    *
>    * Checked integers of different types cannot be used in the same arithmetic expression.
>    *
>    * There are convenience typedefs for all PR integer types, of the following form (these are just 2 examples):

Nix the PR reference here.
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-07 17:37:03 PST
Comment on attachment 602996 [details] [diff] [review]
2. Let CheckedInt support the 3 families of integer types: stdint, standard integer types, and PR types

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

Speaking of mfbt style, I mentioned mfbt/STYLE at one point in here.  I'm not going to go out of my way to point out mfbt style changes needed until this actually moves into the mfbt/ source directory, but you may want to keep them in mind for patches atop these.

I didn't look particularly closely at TestCheckedInt.cpp for reasons of the style thing, although some of the changes did look a little nicer going solely by what Splinter showed me.  I assume the entire file is so hairy that it's all a bit crazy no matter how it happens.

::: xpcom/ds/CheckedInt.h
@@ +41,5 @@
>  #define mozilla_CheckedInt_h
>  
>  #include "prtypes.h"
>  
> +#include <mozilla/Assertions.h>

I think we want this included using "", not <>.  Or at least all existing includes of mfbt headers that I'm aware of use ""...

@@ +47,5 @@
>  #include <climits>
>  
>  namespace mozilla {
>  
>  namespace CheckedInt_internal {

Per mfbt/STYLE, this should be |namespace detail|.

@@ +59,5 @@
>  
> +/*** Step 1: manually record supported types
> + ***
> + *** What's nontrivial here is that there are different families of integer types: plain integer types, stdint types,
> + *** and PR types. It is merrily undefined which types from one family may be just typedefs for a type from another family.

The PR types bit should move to a different header, if this is going to move into mfbt, as Ms2ger notes.

@@ +61,5 @@
> + ***
> + *** What's nontrivial here is that there are different families of integer types: plain integer types, stdint types,
> + *** and PR types. It is merrily undefined which types from one family may be just typedefs for a type from another family.
> + ***
> + *** For example, on GCC 4.6, aside from the 10 'standard types' (including long long types), the only other type

"10 C99 types" seems a little more concise, and it'd implicitly include long long (which is so un-unusual at this point, even ignoring the "lateness" of its standardization, that it hardly seems worth calling out).
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-03-07 22:21:11 PST
Sorry, I must be stupid but what is this mfbt/STYLE document you're referring to?
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2012-03-08 01:58:47 PST
(In reply to Benoit Jacob [:bjacob] from comment #13)
> Sorry, I must be stupid but what is this mfbt/STYLE document you're
> referring to?

You aren't, it only just landed in bug 732321.
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-08 15:57:21 PST
Comment on attachment 602998 [details] [diff] [review]
4. make nonstandard features optional

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

The non-fixed-size types give only weak limit guarantees.  What's the reason there are checked variants for the native types, anyway?  Shouldn't people just be using the fixed-size ones, if they care about overflow, underflow, imprecision, and all that?

::: xpcom/ds/CheckedInt.h
@@ +40,5 @@
>  #ifndef mozilla_CheckedInt_h
>  #define mozilla_CheckedInt_h
>  
> +/*** Build options. Comment out these #defines to disable the corresponding optional feature.
> + *** Disabling features may be useful for code using CheckedInt outside of Mozilla (e.g. WebKit)

So the idea is people can copy this file, then they make their half dozen lines of changes below and leave the rest unchanged?

@@ +47,5 @@
> +// enable support for NSPR types like PRInt64
> +#define CHECKEDINT_ENABLE_PR_INTEGER_TYPES
> +
> +// enable support for long long and unsigned long long
> +#define CHECKEDINT_ENABLE_LONG_LONG

Channeling Andreas Gal, is this really necessary in the year of our Lord 2012?

@@ +50,5 @@
> +// enable support for long long and unsigned long long
> +#define CHECKEDINT_ENABLE_LONG_LONG
> +
> +// enable usage of MOZ_STATIC_ASSERT to check for unsupported types.
> +// If disabled, static asserts are just removed. You'll still get compile errors, just less helpful ones.

Is this the case because of the nature of the tricks used here?  'cause it's certainly not the case generally.

@@ +61,5 @@
> +#ifdef CHECKEDINT_ENABLE_MOZ_ASSERTS
> +    #include <mozilla/Assertions.h>
> +#else
> +    #ifndef MOZ_STATIC_ASSERT
> +        #define MOZ_STATIC_ASSERT(x)

MSA takes (cond, reason), not (cond).
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-08 16:03:43 PST
Comment on attachment 602999 [details] [diff] [review]
5. update CheckedInt docs

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

::: xpcom/ds/CheckedInt.h
@@ +515,5 @@
> +    /** \returns true if the left and right hand sides are valid and have the same value.
> +      *
> +      * Note that these semantics are the reason why we don't offer a operator!=. Indeed, we'd want
> +      * to have a!=b be equivalent to !(a==b) but that would mean that whenever a or b is invalid, a!=b
> +      * is always true, which would be very confusing.

Nice explanation.
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 09:07:52 PDT
Created attachment 607184 [details] [diff] [review]
7: address Jeff's review comments

This patch addresses all the above review comments. I thought it simpler to address them all at once. Also replying inline below about other review comments.

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #11)
> Aren't you missing an #include "mozilla/StandardInteger.h"?  I'd guess this
> only happens to work because of #include bootlegging.

Fixed.

> Nix the PR reference here.

Removed all PR references.



(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #12)
> I think we want this included using "", not <>.  Or at least all existing
> includes of mfbt headers that I'm aware of use ""...

Fixed.

> Per mfbt/STYLE, this should be |namespace detail|.

Fixed. Other mfbt/STYLE adjustments will be in a forthcoming patch.

> The PR types bit should move to a different header, if this is going to move
> into mfbt, as Ms2ger notes.

Removed all support for PR types. This doesn't seem to break the build here. The PR types are the same as the stdint types on my machine. I'll see if it breaks the Windows builds and if it does, I'll adapt user code to use stdint types.

> "10 C99 types" seems a little more concise, and it'd implicitly include long
> long (which is so un-unusual at this point, even ignoring the "lateness" of
> its standardization, that it hardly seems worth calling out).

My issue with 'c99 types' is that stdint.h is part of c99 too. So (following wikipedia) I called these the 'basic types'.

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #15)
> The non-fixed-size types give only weak limit guarantees.  What's the reason
> there are checked variants for the native types, anyway?  Shouldn't people
> just be using the fixed-size ones, if they care about overflow, underflow,
> imprecision, and all that?

You want CheckedInt<int> when just the guarantee that you'll be informed in case of integer overflow, is enough to make you comfortable about int. I think it's reasonable to expect that such situations exist.

Regarding other integer types such as short/long, I would content that there is no legitimate use for them at all. In fact, the Google c++ style guide discourages their usage, allowing only |int| and fixed-size types.

But real-world code does use all sorts of types, and in particular (despite Google style guide) WebKit is an avid user of |long|. Whence this WebKit bug, where I had adapted their copy to allow |long|:
   https://bugs.webkit.org/show_bug.cgi?id=52401

So, to ease sharing code, I suggest we just keep support for char/short/long.

> > +/*** Build options. Comment out these #defines to disable the corresponding optional feature.
> > + *** Disabling features may be useful for code using CheckedInt outside of Mozilla (e.g. WebKit)
> 
> So the idea is people can copy this file, then they make their half dozen
> lines of changes below and leave the rest unchanged?

Yes. In the new patch, there is only one option left, CHECKEDINT_ENABLE_MOZ_ASSERTS. People can comment out this define to remove the dependency on mfbt Assertions.h.

> 
> @@ +47,5 @@
> > +// enable support for NSPR types like PRInt64
> > +#define CHECKEDINT_ENABLE_PR_INTEGER_TYPES
> > +
> > +// enable support for long long and unsigned long long
> > +#define CHECKEDINT_ENABLE_LONG_LONG
> 
> Channeling Andreas Gal, is this really necessary in the year of our Lord
> 2012?

You're right, neither us nor WebKit seems to use CheckedInt<long long>, so I dropped that.

> 
> @@ +50,5 @@
> > +// enable support for long long and unsigned long long
> > +#define CHECKEDINT_ENABLE_LONG_LONG
> > +
> > +// enable usage of MOZ_STATIC_ASSERT to check for unsupported types.
> > +// If disabled, static asserts are just removed. You'll still get compile errors, just less helpful ones.
> 
> Is this the case because of the nature of the tricks used here?  'cause it's
> certainly not the case generally.

It was the case due to details, you're right to feel that was unsafe, so the new patch falls back to assert() so in the worst case this will still be caught at runtime in debug builds.

> MSA takes (cond, reason), not (cond).

fixed.
Comment 18 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 18:17:13 PDT
Created attachment 607403 [details] [diff] [review]
8: finish adapting CheckedInt to mfbt/STYLE

daunting task --- I hope that CheckedInt.h is enough and I can leave the test as-is?
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 18:19:24 PDT
An API question now. The primary goal of CheckedInt is to be safe in every sense, which includes "fool-proof".

Should CheckedInt have an assertion guarding against calling value() without having formerly called valid()?

I feel that it should, but the test for that is going to be tricky to write. In another project of mine, we do "should assert" unit tests by reimplementing asserts as exceptions and catching them.
Comment 20 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 18:22:07 PDT
(In reply to Benoit Jacob [:bjacob] from comment #18)
> Created attachment 607403 [details] [diff] [review]
> 8: finish adapting CheckedInt to mfbt/STYLE

Also note that a few improvements to comments are in this patch. In particular, expanded comment on operator== and removed a remaining mention of PR types.
Comment 21 Benoit Jacob [:bjacob] (mostly away) 2012-03-20 08:49:53 PDT
Created attachment 607571 [details] [diff] [review]
8: move CheckedInt to MFBT (but support for compiled tests is just a stub)

This patch replaces the previous "8: finish adapting CheckedInt to mfbt/STYLE"

It adapts also the test file, TestCheckedInt.cpp, to mfbt/STYLE, moves it to a new mfbt/tests directory, and adds a stub makefile there.

But I ran into a problem: the implementation of compiled unit tests using CPP_UNIT_TESTS, in config/rules.mk, has dependencies that we can't allow in MFBT, e.g. on XPCOM. So I added a new variable, MFBT_UNIT_TESTS, but it's currently unused i.e. the test is currently not compiled. Need help with our buildsystem here.

Aside from that, this seems good to go.
Comment 22 Benoit Jacob [:bjacob] (mostly away) 2012-03-20 11:03:49 PDT
Created attachment 607626 [details] [diff] [review]
8: move CheckedInt to MFBT (with support for compiled tests, but didn't check that test failures give oranges)

Glandium's idea on IRC was the right one: I can set appropriate makefile variables after including rules.mk. Resetting MOZ_GLUE_{,PROGRAM_}LDFLAGS successfully prevents it from trying to link to libmozglue. See mfbt/tests/Makefile.in
Comment 23 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-29 16:47:16 PDT
Comment on attachment 607626 [details] [diff] [review]
8: move CheckedInt to MFBT (with support for compiled tests, but didn't check that test failures give oranges)

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

I assume this adds the test, and builds it and runs it, in a full browser build.  Does it do the same in a JS engine shell build?  Test coverage in browser builds should -- usually -- be sufficient to make sure the code works.  But I'd really rather not rely on that -- I'd rather we be building the tests in shell builds too.  I think this is okay for now, but I think we need a quick followup to hook up the tests to the JS build system as well.  See https://developer.mozilla.org/En/SpiderMonkey/Build_Documentation for information on how to build the JS shell.

As far as style goes, you've picked up most stuff, just a few bits remain.  (Not surprisingly -- I had to update mfbt/STYLE twice while reading this to account for stuff it didn't already discuss.)  Probably this is enough that I should give one more look -- poke me on IRC when you post it, I'll get to it quicker than I got to this.

::: config/rules.mk
@@ +235,5 @@
>  	  done
>  
>  endif # CPP_UNIT_TESTS
>  
> +ifdef MFBT_UNIT_TESTS

Is there a reason that this should go in config/rules.mk?  Seems like it'd be better in mfbt/tests/Makefile.in, since this bit is entirely specific to a single directory.

::: xpcom/ds/CheckedInt.h
@@ +34,5 @@
>   * and other provisions required by the GPL or the LGPL. If you do not delete
>   * the provisions above, a recipient may use your version of this file under
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */

Or you can change to MPL2 here, I don't think it super-matters exactly when this happens within this patch series.

@@ +36,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +/* Provides checked integers, detecting integer overflow and divide-by-0 */

Period at the end of this.

But, come to think of it, the details here aren't complete, are they?  Unary minus presumably also gets detected, too (think |-int8_t(-128)|, say).

@@ +48,1 @@
>   ***/

mfbt uses this style for block comments (and for that matter, I'm not sure I've seen triple-asterisk in Mozilla, outside the files concerned in this patch):

/*
 * Build options.  ...
 */

This applies throughout.

@@ +48,3 @@
>   ***/
>  
>  // enable usage of MOZ_STATIC_ASSERT to check for unsupported types.

Capitalize full sentences, please.

@@ +52,1 @@
>  #define CHECKEDINT_ENABLE_MOZ_ASSERTS

This should be prefixed with MOZ_ to avoid conflicts; I just updated mfbt/STYLE for this.

@@ +86,5 @@
>  
> +template<typename IntegerType>
> +struct IsSupportedPass2
> +{
> +    enum { Value = 0 };

Maybe I'm missing something, but why aren't these enum { Value } instances |static const bool value = false| or such?  (I believe because this is a member field, it'd be |value|, not |Value|.)

At risk of suggesting something crazy, this would be more concise and just as readable (?) if you did them this way:

  struct Supported { static const bool value = true; };
  struct Unsupported { static const bool value = false; };
  
  template<typename IntegerType>
  struct IsSupportedPass2 : Unsupported { };
  
  template<typename IntegerType>
  struct IsSupported : IsSupportedPass2<IntegerType> { };
  
  template<>
  struct IsSupported<int8_t> : Supported { };

...and so on.  Thoughts?

@@ +166,3 @@
>   ***/
>  
> +template<int Size, bool Signedness>

size_t is a better type here than int.  You'll have to #include <stddef.h> for this, but I can't see how that could be a problem.

@@ +215,3 @@
>  };
>  
> +template<typename IntegerType, int size = sizeof(IntegerType)>

size_t is again a better type here, particularly as the type of a sizeof() is size_t.

@@ +234,3 @@
>  {
>      enum {
> +        Value = CHAR_BIT * sizeof(IntegerType) - 1

static const size_t

@@ +245,5 @@
> +      // Bitwise ops may return a larger type, that's why we cast explicitly.
> +      // In C++, left bit shifts on signed values is undefined by the standard
> +      // unless the shifted value is representable.
> +      // notice that signed-to-unsigned conversions are always well-defined in
> +      // the standard, as the value congruent to 2^n as expected. By contrast,

2**n, and capitalize "Notice" since it's a sentence.

@@ +250,5 @@
> +      // unsigned-to-signed is only well-defined if the value is representable.
> +      return IsSigned<IntegerType>::Value
> +                ? IntegerType(typename UnsignedType<IntegerType>::Type(1)
> +                                << PositionOfSignBit<IntegerType>::Value)
> +                : IntegerType(0);

This ternary expression style's a bit funky.  I wrote up mfbt's style and added it to mfbt/STYLE just now -- check it out, apply it to the ternaries here.

@@ +255,4 @@
>      }
>  };
>  
> +template<typename IntegerType> struct MaxValue

template<...>
struct MaxValue

@@ +261,2 @@
>      {
> +      return ~MinValue<IntegerType>::value();

Tricksy.

@@ +269,4 @@
>   ***/
>  
> +// bitwise ops may return a larger type, so it's good to use these inline
> +// helpers guaranteeing that the result is really of type T

Capitalize, and trailing period.

@@ +273,4 @@
>  
> +template<typename T>
> +inline T
> +hasSignBit(T x)

HasSignBit

@@ +277,3 @@
>  {
> +  // in C++, right bit shifts on negative values is undefined by the standard.
> +  // notice that signed-to-unsigned conversions are always well-defined in the

"In C++", "Notice"

@@ +286,5 @@
>  }
>  
> +template<typename T>
> +inline T
> +binaryComplement(T x)

BinaryComplement

@@ +303,5 @@
>  {
>      static bool run(U x)
>      {
> +      return (x <= MaxValue<T>::value()) &&
> +              (x >= MinValue<T>::value());

These don't need to be over-parenthesized.

@@ +324,5 @@
>      {
> +      if (sizeof(T) > sizeof(U))
> +          return 1;
> +      else
> +          return x <= U(MaxValue<T>::value());

else-after-return is no good -- if you've returned, the else is pointless.  I'd say make this a ternary expression.

@@ +336,5 @@
>      {
> +      if (sizeof(T) >= sizeof(U))
> +          return x >= 0;
> +      else
> +          return (x >= 0) && (x <= U(MaxValue<T>::value()));

Same here, and remove the unnecessary parentheses.

@@ +342,5 @@
>  };
>  
> +template<typename T, typename U>
> +inline bool
> +isInRange(U x)

IsInRange

More generally, the freestanding methods should all be capitalized.  I'm not going to bother noting all these.

@@ +355,5 @@
> +  // addition is valid if the sign of x+y is equal to either that of x or that
> +  // of y. Beware! These bitwise operations can return a larger integer type,
> +  // if T was a small type like int8, so we explicitly cast to T.
> +  return IsSigned<T>::Value ?
> +                hasSignBit(binaryComplement(T((result^x) & (result^y))))

result ^ x, result ^ y

I see other instances of these, make sure to search 'em all out.

@@ +367,2 @@
>  {
> +  // substraction is valid if either x and y have same sign, or x-y and x have

Subtraction (note spelling, too)

@@ +367,3 @@
>  {
> +  // substraction is valid if either x and y have same sign, or x-y and x have
> +  // same sign

Period.

@@ +374,4 @@
>  }
>  
>  template<typename T,
> +         bool IsSigned =  IsSigned<T>::Value,

One space after =.

@@ +375,5 @@
>  
>  template<typename T,
> +         bool IsSigned =  IsSigned<T>::Value,
> +         bool TwiceBiggerTypeIsSupported
> +           = IsSupported<typename TwiceBiggerType<T>::Type>::Value>

= at end of the previous line, please.

@@ +400,2 @@
>  
> +      if (x == 0 || y == 0) return true;

if (x == 0 || y == 0)
  return true;

@@ +409,5 @@
> +          if (y > 0)
> +              return x >= min / y;
> +          else
> +              return y >= max / x;
> +      }

These blocks need two-space indentation.  Also there's a bit of else-after-return-statement here.

@@ +419,5 @@
>  {
>      static bool run(T x, T y)
>      {
> +      const T max = MaxValue<T>::value();
> +      if (x == 0 || y == 0) return true;

if (...)
  return true;

@@ +426,5 @@
>  };
>  
> +template<typename T>
> +inline bool
> +isMulValid(T x, T y, T /*result not used*/)

/* result not used */

@@ +463,2 @@
>  
>  } // end namespace detail

Just // namespace detail

@@ +467,5 @@
>  /*** Step 4: Now define the CheckedInt class.
>   ***/
>  
>  /** \class CheckedInt
>    * \brief Integer wrapper class checking for integer overflow and other errors

/**
 *
 *

for the comment style.

mfbt hasn't used \class \brief \yadda comment style before.  What tool consumes this style?  Is some tool actually being used to consume it?  I'm inclined to say we should just comment things without faux markup (except maybe @param javadoc-y stuff because all our IDL files already use it and it's minimal).

@@ +473,5 @@
>    *            - any basic integer type such as |int|
>    *            - any stdint type such as |int8_t|
>    *
> +  * This class implements guarded integer arithmetic. Do a computation, check
> +  * that valid() returns true, you then have a guarantee that no problem, such 

You mentioned that value() doesn't assert valid() right now.  I'm pretty sure that's something we should be doing -- if this doesn't assert, it'll be too easy to accidentally misuse it.  That said, how much existing code depends on the lack of assertion here?  That seems the biggest stumbling block to making this idiot-proof in debug builds.  :-)  I can also imagine patterns like outparam-plus-success bool that might have issues with this:

  bool
  CanMultiply(uint8_t x, uint8_t y, uint8_t* product)
  {
    CheckedUint8 p = CheckedUint8(x) * y;
    *product = p.value();
    return p.valid();
  }

Something like an extract(bool* valid, T* value) method might be a solution to this problem (if it even is a problem).

But perhaps we should get things landed first, before deciding on the valid()/value() semantics.  We can always fix users if we decide to change things.

@@ +492,1 @@
>      }

Heh, and here's an example of the pattern I posited above.

@@ +555,1 @@
>        */

/** 
 *
 */

comment style.  Looks like there are a few others of this later on, too.

@@ +700,3 @@
>  };
>  
> +#define CHECKEDINT_BASIC_BINARY_OPERATOR(NAME, OP)                    \

MOZ_* for one, and for two, #undef this once you're done using it.

@@ +707,5 @@
> +  T x = lhs.mValue;                                                   \
> +  T y = rhs.mValue;                                                   \
> +  T result = x OP y;                                                  \
> +  T isOpValid                                                         \
> +      = detail::is##NAME##Valid(x, y, result);                      \

= on the previous line.

@@ +708,5 @@
> +  T y = rhs.mValue;                                                   \
> +  T result = x OP y;                                                  \
> +  T isOpValid                                                         \
> +      = detail::is##NAME##Valid(x, y, result);                      \
> +  /* give the compiler a good chance to perform RVO */                \

Capitalize, period.

I know what RVO is.  I'm not entirely sure if the typical reader would.  I guess this is crazy enough code it's probably not worth trying to clarify.

@@ +727,5 @@
> +  T x = lhs.mValue;
> +  T y = rhs.mValue;
> +  bool isOpValid = detail::isDivValid(x, y);
> +  T result = isOpValid ? (x / y) : 0;
> +  /* give the compiler a good chance to perform RVO */

Same again.

@@ +761,3 @@
>  }
>  
> +#define CHECKEDINT_CONVENIENCE_BINARY_OPERATORS(OP, COMPOUND_OP)  \

MOZ_* and #undef

@@ +807,5 @@
>  typedef CheckedInt<uint32_t> CheckedUint32;
>  typedef CheckedInt<int64_t>  CheckedInt64;
>  typedef CheckedInt<uint64_t> CheckedUint64;
>  
>  } // end namespace mozilla

Just // namespace mozilla

::: mfbt/tests/Makefile.in
@@ +32,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

Use the MPL2 header.

@@ +46,5 @@
> +MODULE = mozglue
> +STL_FLAGS =
> +
> +MFBT_UNIT_TESTS = \
> +  TestCheckedInt.cpp \

It's slightly more autocomplete-friendly to name this CheckedIntTest.cpp.  Or, you're in a tests/ directory already, so you could get rid of "Test" entirely.  Don't much care which, myself.

::: mfbt/tests/TestCheckedInt.cpp
@@ +33,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

MPL2

@@ +36,5 @@
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +
> +#include "mozilla/CheckedInt.h"

You only need one blank line before this, not two.

@@ +53,5 @@
> +int gTestsFailed = 0;
> +
> +void verifyImplFunction(bool x, bool expected,
> +                          const char* file, int line,
> +                          int size, bool isTSigned)

These two lines seem mis-indented.
Comment 24 Benoit Jacob [:bjacob] (mostly away) 2012-03-29 19:08:16 PDT
Thanks for the big review.

Just a couple replies for now: I am swamped in unrelated stuff.

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #23)
> But, come to think of it, the details here aren't complete, are they?  Unary
> minus presumably also gets detected, too (think |-int8_t(-128)|, say).

This falls in the "integer overflow" category too! As it's an overflow in the substraction,

    0 - x

where x = int8_t(-128).

That's why all the bad cases in integer arithmetic are either integer overflow or divide-by-zero.


> @@ +86,5 @@
> >  
> > +template<typename IntegerType>
> > +struct IsSupportedPass2
> > +{
> > +    enum { Value = 0 };
> 
> Maybe I'm missing something, but why aren't these enum { Value } instances
> |static const bool value = false| or such?  (I believe because this is a
> member field, it'd be |value|, not |Value|.)
> 
> At risk of suggesting something crazy, this would be more concise and just
> as readable (?) if you did them this way:
> 
>   struct Supported { static const bool value = true; };
>   struct Unsupported { static const bool value = false; };
>   
>   template<typename IntegerType>
>   struct IsSupportedPass2 : Unsupported { };
>   
>   template<typename IntegerType>
>   struct IsSupported : IsSupportedPass2<IntegerType> { };
>   
>   template<>
>   struct IsSupported<int8_t> : Supported { };
> 
> ...and so on.  Thoughts?

First of all, this is not crazy at all, since that's exactly how the GNU libstdc++ is implemented.

Here's a question for you: static const has always made me very uncomfortable for two reasons:

1) it's undefined what happens if one tries to effectively mutate a static const int by doing:

   static const int x;
   int* p = const_cast<int*>(&x);
   *p = 123;

On most modern platforms, this will segfault because x is stored in a read-only segment, but in other platforms (yay MSDOS) I bet that this will just work.

2) it's not guaranteed that a static const is a pure compile-time value and has zero cost (doesn't even exist) at runtime. I guess that compilers are good at optimizing this, since e.g. the GNU libstdc++ uses static const's extensively, but given that enum values give me exactly what I want, why not use them?

I guess the reason not to use enum values is lack of typing. So if, considering all this, you still prefer good typed static const's, I can sympathize with that.

AFAICS, C++11 offers two better ways to do pure compile-time constants:
 - typed enum values
 - constexpr functions, so one could do:  static constexpr int Value();
Comment 25 :Ms2ger (⌚ UTC+1/+2) 2012-03-30 02:01:41 PDT
Comment on attachment 607626 [details] [diff] [review]
8: move CheckedInt to MFBT (with support for compiled tests, but didn't check that test failures give oranges)

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

::: xpcom/ds/CheckedInt.h
@@ +15,5 @@
>   *
>   * The Original Code is Mozilla code.
>   *
>   * The Initial Developer of the Original Code is the Mozilla Corporation.
> + * Portions created by the Initial Developer are Copyright (C) 2010-2012

Even if you don't use MPL2 in this patch, don't change this.

@@ +467,5 @@
>  /*** Step 4: Now define the CheckedInt class.
>   ***/
>  
>  /** \class CheckedInt
>    * \brief Integer wrapper class checking for integer overflow and other errors

doxygen, fwiw

::: mfbt/tests/TestCheckedInt.cpp
@@ +59,5 @@
> +  if (x == expected) {
> +    gTestsPassed++;
> +  } else {
> +    gTestsFailed++;
> +    std::cerr << "Test failed at " << file << ":" << line;

:/ Followup to make this use TestHarness.h?

@@ +130,5 @@
> +  // could potentially NOT be caught by any other tests... while making everything wrong!
> +
> +  T bit = 1;
> +  for(unsigned int i = 0; i < sizeof(T) * CHAR_BIT - 1; i++)
> +  {

for (...) {

::: xpcom/tests/TestCheckedInt.cpp
@@ -1,1 @@
> -/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

This should be hg mv'ed (hg addrem -s might be happy to pick it up)
Comment 26 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-09 16:25:18 PDT
(In reply to Benoit Jacob [:bjacob] from comment #24)
> 1) it's undefined what happens if one tries to effectively mutate a static
> const int by doing:
> 
>    static const int x;
>    int* p = const_cast<int*>(&x);
>    *p = 123;
> 
> On most modern platforms, this will segfault because x is stored in a
> read-only segment, but in other platforms (yay MSDOS) I bet that this will
> just work.

I'm not too worried about crazy non-modern platforms.  :-)  If they have problems, that's their problem.  Plus for this bit, I think the major platforms will provide much of the same testing as basically-good-enough backup for oddball platforms.

Not to mention they'll have to be const_cast<>ing away this stuff, and reaching into a detail namespace moreover.  That's willful misbehavior, which worries me much less than accidental mistakes.  I can't see how stuff here might trigger accidental problems.

> 2) it's not guaranteed that a static const is a pure compile-time value and
> has zero cost (doesn't even exist) at runtime. I guess that compilers are
> good at optimizing this, since e.g. the GNU libstdc++ uses static const's
> extensively, but given that enum values give me exactly what I want, why not
> use them?
> 
> I guess the reason not to use enum values is lack of typing. So if,
> considering all this, you still prefer good typed static const's, I can
> sympathize with that.

It's a bit of the better typing, and a bit of the typing making the concepts clearer.  Given that modern compilers (that we care about) are smart enough to get this right, I think readability trumps absolute speed for the weird cases.

> AFAICS, C++11 offers two better ways to do pure compile-time constants:
>  - typed enum values
>  - constexpr functions, so one could do:  static constexpr int Value();

C++11 constexpr can be applied to values.  So, if we were guaranteed to be using only compilers that supported constexpr, we could do s/const/constexpr/ on these and be guaranteed compile-time constancy.  We might consider adding MOZ_CONSTEXPR or somesuch and letting people use that for this use case, perhaps.  Although as constexpr changes semantics just beyond making some things into compile-time constants (e.g. you can use the result of a properly-called constexpr function as an array bound, say, without it being a C99 variable-length array or a compile error), I'm a bit leery of doing that yet.
Comment 27 Benoit Jacob [:bjacob] (mostly away) 2012-04-20 11:40:38 PDT
(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #26)
> (In reply to Benoit Jacob [:bjacob] from comment #24)
> > 1) it's undefined what happens if one tries to effectively mutate a static
> > const int by doing:
> > 
> >    static const int x;
> >    int* p = const_cast<int*>(&x);
> >    *p = 123;
> > 
> > On most modern platforms, this will segfault because x is stored in a
> > read-only segment, but in other platforms (yay MSDOS) I bet that this will
> > just work.
> 
> I'm not too worried about crazy non-modern platforms.  :-)  If they have
> problems, that's their problem.  Plus for this bit, I think the major
> platforms will provide much of the same testing as basically-good-enough
> backup for oddball platforms.
> 
> Not to mention they'll have to be const_cast<>ing away this stuff, and
> reaching into a detail namespace moreover.  That's willful misbehavior,
> which worries me much less than accidental mistakes.  I can't see how stuff
> here might trigger accidental problems.
> 
> > 2) it's not guaranteed that a static const is a pure compile-time value and
> > has zero cost (doesn't even exist) at runtime. I guess that compilers are
> > good at optimizing this, since e.g. the GNU libstdc++ uses static const's
> > extensively, but given that enum values give me exactly what I want, why not
> > use them?
> > 
> > I guess the reason not to use enum values is lack of typing. So if,
> > considering all this, you still prefer good typed static const's, I can
> > sympathize with that.
> 
> It's a bit of the better typing, and a bit of the typing making the concepts
> clearer.  Given that modern compilers (that we care about) are smart enough
> to get this right, I think readability trumps absolute speed for the weird
> cases.
> 
> > AFAICS, C++11 offers two better ways to do pure compile-time constants:
> >  - typed enum values
> >  - constexpr functions, so one could do:  static constexpr int Value();
> 
> C++11 constexpr can be applied to values.

What I didn't realize is that indeed, for static member variables in class scope, constexpr does indeed turn them into true compile-time constants with an address (the same is not true for local variables in function scope).

$ cat const.cpp
struct foo {
  static constexpr int i = 0;
};

int main()
{
  const int *p = &(foo::i);
}

$ g++ const.cpp --std=c++0x -o c && ./c
/tmp/ccVAISn6.o:const.cpp:function main: error: undefined reference to 'foo::i'
collect2: ld returned 1 exit status

The last advantage I can see for enums is that they give compile-time instead of link-time errors. So would constexpr getter functions.
Comment 28 Benoit Jacob [:bjacob] (mostly away) 2012-05-01 15:55:17 PDT
Created attachment 620109 [details] [diff] [review]
8: move CheckedInt to MFBT, enable unit tests in mfbt/tests

Here you go: this seems to be working. unit tests enabled in mfbt and confirmed to be working: if I introduce a failure in the test, |make check| actually fails.

For the assertion on validity: I went with having CheckedInt::value() assert that the checkedint is valid. That should not cause trouble, though I'm waiting for try results:
https://tbpl.mozilla.org/?tree=Try&rev=4e1d005d97c5

My initial idea was different: I was wondering if we could have value() assert that valid() has been called. This way we would catch forgot-to-call-valid bugs even without actually reproducing a integer overflow and even in release builds. But that would be more work and overhead. In the end I like the current solution better. We just need to make sure that our unit tests do test inputs that are prone to causing integer overflows. The WebGL conformance tests do.
Comment 29 Benoit Jacob [:bjacob] (mostly away) 2012-05-01 15:59:28 PDT
Remaining questions:

CheckedInt exposes value() and valid() methods.

Do you want them capitalized as Value() and Valid()?

Should I rename Valid() to something more different from Value() to avoid risk of confusion?
Comment 30 Benoit Jacob [:bjacob] (mostly away) 2012-05-01 16:02:44 PDT
Android build failure in mfbt/tests, cannot find -lmozglue. Any clue?
Comment 31 Benoit Jacob [:bjacob] (mostly away) 2012-05-01 16:04:36 PDT
Removed MODULE=mozglue from the Makefile. New try:
https://tbpl.mozilla.org/?tree=Try&rev=d36d95c22f4f
Comment 32 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-09 16:26:21 PDT
Comment on attachment 620109 [details] [diff] [review]
8: move CheckedInt to MFBT, enable unit tests in mfbt/tests

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

> Do you want them capitalized as Value() and Valid()?

Nope, they're not standalone functions, so camelCaps is right.

> Should I rename Valid() to something more different from Value() to avoid risk of confusion?

A fair concern.  How about isValid()?

::: gfx/2d/Blur.cpp
@@ +80,5 @@
>      if (boxSize == 1) {
>          memcpy(aOutput, aInput, aWidth*aRows);
>          return;
>      }
> +    uint32_t reciprocal = (uint64_t(1) << 32)/boxSize;

Spaces around the /.

@@ +127,5 @@
>              int32_t tmp = x - aLeftLobe;
>              int32_t last = max(tmp, 0);
>              int32_t next = min(tmp + boxSize, aWidth - 1);
>  
> +            aOutput[aWidth * y + x] = (uint64_t(alphaSum)*reciprocal) >> 32;

Spaces around the *.

@@ +158,5 @@
>      if (boxSize == 1) {
>          memcpy(aOutput, aInput, aWidth*aRows);
>          return;
>      }
> +    uint32_t reciprocal = (uint64_t(1) << 32)/boxSize;

Spaces.

@@ +198,5 @@
>              int32_t tmp = y - aTopLobe;
>              int32_t last = max(tmp, 0);
>              int32_t next = min(tmp + boxSize, aRows - 1);
>  
> +            aOutput[aWidth * y + x] = (uint64_t(alphaSum)*reciprocal) >> 32;

Spaces.

::: xpcom/ds/CheckedInt.h
@@ +187,3 @@
>  };
>  
> +template<typename IntegerType, size_t size = sizeof(IntegerType)>

Size, not size, because it's a template parameter.

@@ +215,5 @@
> +      // Bitwise ops may return a larger type, that's why we cast explicitly.
> +      // In C++, left bit shifts on signed values is undefined by the standard
> +      // unless the shifted value is representable.
> +      // Notice that signed-to-unsigned conversions are always well-defined in
> +      // the standard, as the value congruent to 2**n as expected. By contrast,

This sentence parses a little weirdly for me, I think because there are two clauses after the comma.  Maybe move the comma after "2**n"?

@@ +269,5 @@
>  
> +template<typename T,
> +         typename U,
> +         bool isTSigned = IsSigned<T>::value,
> +         bool isUSigned = IsSigned<U>::value>

These parameters should be capitalized.

@@ +278,2 @@
>  {
> +    static bool Run(U x)

The run() methods should be lowercased, through the rest of this.

@@ +310,5 @@
>      {
> +      if (sizeof(T) >= sizeof(U))
> +          return x >= 0;
> +      else
> +          return (x >= 0) && (x <= U(MaxValue<T>::value()));

Either get rid of the else here, or use a ternary.

@@ +427,4 @@
>  };
>  template<typename T>
> +inline T
> +oppositeIfSigned(T x)

Capitalize.

@@ +436,4 @@
>  
>  
> +/* Step 4: Now define the CheckedInt class.
> + */

/*
 * Step 4...
 */

@@ +503,4 @@
>  template<typename T>
>  class CheckedInt
>  {
>  protected:

Indent this two spaces.

@@ +515,4 @@
>      }
>  
> +  public:
> +    /** Constructs a checked integer with given @a value. The checked integer is

/**
 * Constructs...

All multi-line C-style comments should start with a blank line.  If that wasn't clear in mfbt/STYLE, let me know, and I'll make it clearer.

@@ +560,5 @@
> +    /** @returns the sum. Checks for overflow. */
> +    template<typename U>
> +    friend CheckedInt<U> operator +(const CheckedInt<U>& lhs,
> +                                    const CheckedInt<U>& rhs);
> +    /** Adds. Checks for overflow. @returns self reference */

I'm kind of shuddering thinking about the definition of an operator which, built-in, returns a self reference but where a user-defined operator doesn't return a self reference.  I wouldn't bother mentioning the self reference bit, because I'm pretty sure we would forbid the opposite situation.  :-)

@@ +567,5 @@
> +    /** @returns the difference. Checks for overflow. */
> +    template<typename U>
> +    friend CheckedInt<U> operator -(const CheckedInt<U>& lhs,
> +                                    const CheckedInt<U> &rhs);
> +    /** Substracts. Checks for overflow. @returns self reference */

typo

@@ +570,5 @@
> +                                    const CheckedInt<U> &rhs);
> +    /** Substracts. Checks for overflow. @returns self reference */
> +    template<typename U>
> +    CheckedInt& operator -=(U rhs);
> +    /** @returns the product. Checks for overflow. */

Is it particularly helpful to document this?  I'd expect any user to infer reasonable descriptions for all the standard mathematical operators, in which case it seems like just extra noise to say anything here.  Not saying you should remove them, just that I don't see the value they add, offhand.

@@ +577,5 @@
> +                                    const CheckedInt<U> &rhs);
> +    /** Multiplies. Checks for overflow. @returns self reference */
> +    template<typename U>
> +    CheckedInt& operator *=(U rhs);
> +    /** @returns the quotient. Checks for overflow and for divide-by-zero. */

I could go either way on calling out divide-by-zero -- seems like the sort of thing where people won't immediately think of it as an error, but if they think of it it'll definitely be considered one.  Meh.

@@ +592,3 @@
>      CheckedInt operator -() const
>      {
> +      // circumvent msvc warning about - applied to unsigned int.

Capitalize.

@@ +595,5 @@
> +      // if we're unsigned, the only valid case anyway is 0
> +      // in which case - is a no-op.
> +      T result = detail::oppositeIfSigned(mValue);
> +      /* Give the compiler a good chance to perform RVO
> +       * (return value optimization).

/*
 * Give...

Or just fit it all on one line, if you can, perhaps massaging wording.

@@ +628,5 @@
>  
>      /** prefix ++ */
>      CheckedInt& operator++()
>      {
> +      *this = *this + 1;

My instinct is to change this to |*this +=1;|, although perhaps for how messily complicated this class is, this form is preferable.  Same for other instances, and -- too.  Up to you.

@@ +657,4 @@
>      }
>  
> +  private:
> +    /** operator!= is disabled: see the comment on operator== */

I'd comment once for all of these, keep wordiness down.

@@ +683,5 @@
> +  T y = rhs.mValue;                                                   \
> +  T result = x OP y;                                                  \
> +  T isOpValid                                                         \
> +      = detail::Is##NAME##Valid(x, y, result);                        \
> +  /* Give the compiler a good chance to perform RVO                   \

/*
 * Give...

@@ +697,4 @@
>  
> +#undef MOZ_CHECKEDINT_BASIC_BINARY_OPERATOR
> +
> +// division can't be implemented by MOZ_CHECKEDINT_BASIC_BINARY_OPERATOR

Division

@@ +713,3 @@
>  }
>  
> +// implement castToCheckedInt<T>(x), making sure that

Implement

@@ +723,3 @@
>  {
> +  typedef CheckedInt<T> returnType;
> +  static CheckedInt<T> Run(U u) { return u; }

Sigh, this was fine as it was, indented four (two for the implied public/private/protected level, two for members).

@@ +730,3 @@
>  {
> +  typedef const CheckedInt<T>& returnType;
> +  static const CheckedInt<T>& Run(const CheckedInt<T>& u) { return u; }

Same.

::: mfbt/tests/Makefile.in
@@ +18,5 @@
> +  $(NULL)
> +
> +DIRS =
> +
> +ifdef MFBT_UNIT_TESTS

Why the ifdef?

::: mfbt/tests/TestCheckedInt.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/CheckedInt.h"
> +#include <iostream>

Blank line between these.

@@ +10,5 @@
> +#ifndef MOZ_CHECKEDINT_ENABLE_MOZ_ASSERTS
> +#  error MOZ_CHECKEDINT_ENABLE_MOZ_ASSERTS should be defined by CheckedInt.h
> +#endif
> +
> +using namespace mozilla::detail;

I'd prefer you didn't do this, rather had |using mozilla::detail::Foo| for each |Foo| you need -- more explicit, less worry about over-dependence on detail occurring.

@@ +44,5 @@
> +#define VERIFY_IS_VALID(x)   VERIFY_IMPL((x).valid(), true)
> +#define VERIFY_IS_INVALID(x) VERIFY_IMPL((x).valid(), false)
> +#define VERIFY_IS_VALID_IF(x,condition) VERIFY_IMPL((x).valid(), (condition))
> +
> +template<typename T, unsigned int size = sizeof(T)>

size_t seems a better type, no?  And |Size|, too.

@@ +74,5 @@
> +  static bool alreadyRun = false;
> +  // integer types from different families may just be typedefs for types from other families.
> +  // e.g. int32_t might be just a typedef for int. No point re-running the same tests then.
> +  if (alreadyRun)
> +      return;

Two spaces.

@@ +78,5 @@
> +      return;
> +  alreadyRun = true;
> +
> +  VERIFY(IsSupported<T>::value);
> +  enum{ isTSigned = IsSigned<T>::value };

enum {

@@ +91,5 @@
> +
> +  const CheckedInt<T> max(MaxValue<T>::value());
> +  const CheckedInt<T> min(MinValue<T>::value());
> +
> +  // check min() and max(), since they are custom implementations and a mistake there

Check

@@ +96,5 @@
> +  // could potentially NOT be caught by any other tests... while making everything wrong!
> +
> +  T bit = 1;
> +  for(unsigned int i = 0; i < sizeof(T) * CHAR_BIT - 1; i++)
> +  {

for (...) {

@@ +112,5 @@
> +
> +  /* addition / substraction checks */
> +
> +  VERIFY_IS_VALID(zero+zero);
> +  VERIFY(zero+zero == zero);

zero + zero, zero + one and one + one elsewhere
Comment 33 Benoit Jacob [:bjacob] (mostly away) 2012-05-13 13:22:32 PDT
Created attachment 623519 [details] [diff] [review]
8: (UPDATED) move CheckedInt to MFBT, enable unit tests in mfbt/tests

Updated per review comments. Carrying forward r+.

Applied all of your review comments AFAICR.

Removed the trivial comments on arithmetic operators. My primary reason for adding such comments was a habit from using Doxygen: with doxygen, functions without explicit documentation look weird. But that's an issue with Doxygen which we don't even use.

I'm sorry that this code seems "messily complicated" to you, as that means it will seem even worse to the average user. I had tried to keep it as simple as possible as predictability and transparency were key design goals, e.g. the reason why I didn't want to use expression templates.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=2c0fe2b77a89
Comment 34 :Ms2ger (⌚ UTC+1/+2) 2012-05-13 13:48:28 PDT
Comment on attachment 623519 [details] [diff] [review]
8: (UPDATED) move CheckedInt to MFBT, enable unit tests in mfbt/tests

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

More nits!

::: content/base/src/nsDOMBlobBuilder.h
@@ +43,2 @@
>  
>  #include "mozilla/StandardInteger.h"

Should move the include below the blank line here

::: content/base/src/nsDOMFile.cpp
@@ +65,2 @@
>  #include "nsJSUtils.h"
>  #include "mozilla/Preferences.h"

Right above the Preferences one here.

::: content/canvas/src/WebGLContext.cpp
@@ +424,5 @@
>  
>      // If incrementing the generation would cause overflow,
>      // don't allow it.  Allowing this would allow us to use
>      // resource handles created from older context generations.
> +    if (!(mGeneration+1).isValid())

Spaces, while you're here.

::: content/canvas/src/WebGLContext.h
@@ +68,3 @@
>  #include "nsDataHashtable.h"
>  
>  #include "mozilla/dom/ImageData.h"

Right above ImageData

@@ +2327,5 @@
>      }
>  
>      bool NextGeneration()
>      {
> +        if (!(mGeneration+1).isValid())

Same

::: content/canvas/src/WebGLContextValidate.cpp
@@ +40,5 @@
>  #include "WebGLContext.h"
>  
>  #include "mozilla/Preferences.h"
>  
> +#include "mozilla/CheckedInt.h"

Drop the empty line

::: content/canvas/src/nsCanvasRenderingContext2D.cpp
@@ +107,5 @@
>  #include "CanvasUtils.h"
>  #include "nsIMemoryReporter.h"
>  #include "nsStyleUtil.h"
>  #include "CanvasImageCache.h"
> +#include "mozilla/CheckedInt.h"

This one should go right below mozilla/Assertions.h

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +103,5 @@
>  #include "CanvasUtils.h"
>  #include "nsIMemoryReporter.h"
>  #include "nsStyleUtil.h"
>  #include "CanvasImageCache.h"
> +#include "mozilla/CheckedInt.h"

id.

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +40,5 @@
>  #include "mozilla/Base64.h"
>  #include "nsNetUtil.h"
>  #include "prmem.h"
>  #include "nsDOMFile.h"
> +#include "mozilla/CheckedInt.h"

Below Base64.h

::: content/media/VideoUtils.h
@@ +44,5 @@
>  #include "nsRect.h"
>  #include "nsIThreadManager.h"
>  #include "nsThreadUtils.h"
>  
> +#include "mozilla/CheckedInt.h"

Above ReentrantMonitor.h

::: content/media/webm/nsWebMReader.cpp
@@ +460,5 @@
>    if (tstamp_frames.value() > decoded_frames.value()) {
>  #ifdef DEBUG
>      CheckedInt64 usecs = FramesToUsecs(tstamp_frames.value() - decoded_frames.value(), rate);
>      LOG(PR_LOG_DEBUG, ("WebMReader detected gap of %lld, %lld frames, in audio stream\n",
> +      usecs.isValid() ? usecs.value(): -1,

A space before the ':', please

::: xpcom/ds/CheckedInt.h
@@ +297,5 @@
>  {
>      static bool run(U x)
>      {
> +      return sizeof(T) > sizeof(U)
> +             ? 1

true

@@ +327,4 @@
>  {
> +  // Addition is valid if the sign of x+y is equal to either that of x or that
> +  // of y. Beware! These bitwise operations can return a larger integer type,
> +  // if T was a small type like int8, so we explicitly cast to T.

int8_t

@@ +371,3 @@
>  
> +      if (x == 0 || y == 0)
> +        return true;

Does it make sense to swap these blocks?

@@ +391,5 @@
>      static bool run(T x, T y)
>      {
> +      return x == 0 || y == 0
> +             ? true
> +             : x <= MaxValue<T>::value() / y;

I don't like this pattern much... What's wrong with

return x == 0 || y == 0 || x <= MaxValue<T>::value() / y;

?

@@ +534,4 @@
>      template<typename U>
>      CheckedInt(U value)
> +      : mValue(T(value)),
> +        mIsValid(detail::IsInRange<T>(value))

, at the start of the next line

@@ +555,3 @@
>      }
>  
> +    /** @returns true if the checked integer is valid, i.e. is not the result

/**
 * @returns

@@ +598,3 @@
>      }
>  
> +    /** @returns true if the left and right hand sides are valid

id.

@@ +698,5 @@
> +  bool isOpValid = detail::IsDivValid(x, y);
> +  T result = isOpValid ? (x / y) : 0;
> +  /* give the compiler a good chance to perform RVO */
> +  return CheckedInt<T>(result,
> +                        lhs.mIsValid && rhs.mIsValid && isOpValid);

Indentation of this last line

::: mfbt/tests/Makefile.in
@@ +25,5 @@
> +
> +# ...and run them the usual way
> +check::
> +	@$(EXIT_ON_ERROR) \
> +	  for f in $(subst .cpp,$(BIN_SUFFIX),$(MFBT_UNIT_TESTS)); do \

Hmm, can't you share this with the replacement you do for SIMPLE_PROGRAMS?
Comment 35 :Ms2ger (⌚ UTC+1/+2) 2012-05-13 13:53:06 PDT
Oh, and make it build :)
Comment 36 Benoit Jacob [:bjacob] (mostly away) 2012-05-14 11:18:37 PDT
Created attachment 623731 [details] [diff] [review]
8: (v3) move CheckedInt to MFBT, enable unit tests in mfbt/tests

Updated again to apply Ms2ger's comments, except these:

> 
> @@ +371,3 @@
> >  
> > +      if (x == 0 || y == 0)
> > +        return true;
> 
> Does it make sense to swap these blocks?

For optimization? I don't know. For sure the main case is where x>0 and y>0, but the x==0||y==0 case is cheaper to handle and realistically common too.

> 
> @@ +534,4 @@
> >      template<typename U>
> >      CheckedInt(U value)
> > +      : mValue(T(value)),
> > +        mIsValid(detail::IsInRange<T>(value))
> 
> , at the start of the next line

Not according to mfbt/STYLE. But FWIW I agree with you on that.

> ::: mfbt/tests/Makefile.in
> @@ +25,5 @@
> > +
> > +# ...and run them the usual way
> > +check::
> > +	@$(EXIT_ON_ERROR) \
> > +	  for f in $(subst .cpp,$(BIN_SUFFIX),$(MFBT_UNIT_TESTS)); do \
> 
> Hmm, can't you share this with the replacement you do for SIMPLE_PROGRAMS?

Still trying to understand the basics here.
Comment 37 Benoit Jacob [:bjacob] (mostly away) 2012-05-14 12:33:42 PDT
Created attachment 623769 [details] [diff] [review]
8: (v4) move CheckedInt to MFBT, enable unit tests in mfbt/tests

Unit tests work now, thanks to glandium. The key was LIBS=
Comment 40 Benoit Jacob [:bjacob] (mostly away) 2012-05-14 13:15:34 PDT
There were at least two issues with
  https://hg.mozilla.org/integration/mozilla-inbound/diff/345ae68f15f4/mfbt/tests/Makefile.in

1. still got the 'cannot find -lmozglue' error which caused the backout. Can't reproduce it locally.
2. |make check| doesn't do anything anymore. It used to work with the earlier iteration that had a check:: rule.
Comment 41 Mike Hommey [:glandium] 2012-05-14 13:33:30 PDT
Ah, non Linux need MOZ_GLUE_LDFLAGS =
Comment 42 Benoit Jacob [:bjacob] (mostly away) 2012-05-14 13:37:47 PDT
And 2. was a PEBKAC, sorry. Thanks for comment 41. Retrying
Comment 43 Benoit Jacob [:bjacob] (mostly away) 2012-05-14 13:43:01 PDT
https://tbpl.mozilla.org/?tree=Try&rev=949706c3a23f
Comment 44 Benoit Jacob [:bjacob] (mostly away) 2012-05-14 14:43:11 PDT
Still the same 'cannot find -lmozglue' on android:

https://tbpl.mozilla.org/php/getParsedLog.php?id=11736158&tree=Try&full=1

the Makefile.in:

https://hg.mozilla.org/try/file/949706c3a23f/mfbt/tests/Makefile.in
Comment 45 Mike Hommey [:glandium] 2012-05-14 22:22:34 PDT
Try putting both MOZ_GLUE_* variables before including rules.mk.
Comment 46 Benoit Jacob [:bjacob] (mostly away) 2012-05-15 05:25:45 PDT
https://tbpl.mozilla.org/?tree=Try&rev=82ff472a19de
Comment 48 Benoit Jacob [:bjacob] (mostly away) 2012-05-15 05:59:27 PDT
Another try with WRAP_LDFLAGS= following Mike's advice:
https://tbpl.mozilla.org/?tree=Try&rev=a6cf7997b1ed
Comment 50 Benoit Jacob [:bjacob] (mostly away) 2012-05-15 10:55:33 PDT
Created attachment 624106 [details] [diff] [review]
9: some further tweaks

Some more tweaks. The main one is that in MinValue and MaxValue, contrary to everywhere else, |value| was a static method instead of a static constant. That was an artifact of when I was using enum values for this kind of thing: I wanted to avoid enum values there as I needed explicitly the integer type at hand. But since we moved to static constants, this can be a static constant too.

The other tweak is another ( x?true:y ) ----> ( x||y ) change.
Comment 51 Benoit Jacob [:bjacob] (mostly away) 2012-05-15 13:05:34 PDT
Proof that compiled unit test failures do give oranges!
https://tbpl.mozilla.org/?tree=Try&rev=206a40330d6b
Comment 52 :Ms2ger (⌚ UTC+1/+2) 2012-05-15 13:38:55 PDT
Comment on attachment 624106 [details] [diff] [review]
9: some further tweaks

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

lgtm, assuming Waldo is fine with the value thing.

::: mfbt/CheckedInt.h
@@ +406,1 @@
>           : y != 0;

Actually, this might be clearer as

y != 0 && (!IsSigned<T>::value || x != MinValue<T>::value || y != T(-1))

or even

y != 0 && !(IsSigned<T>::value && x == MinValue<T>::value && y == T(-1))

::: mfbt/tests/TestCheckedInt.cpp
@@ +393,5 @@
>      VERIFY_IS_VALID(CheckedInt<T>(U(100))); \
>      if (isUSigned) \
>        VERIFY_IS_VALID_IF(CheckedInt<T>(U(-1)), isTSigned); \
>      if (sizeof(U) > sizeof(T)) \
> +      VERIFY_IS_INVALID(CheckedInt<T>(U(detail::MaxValue<T>::value)+1)); \

Spaces around + while you're here
Comment 53 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-15 15:01:24 PDT
Comment on attachment 624106 [details] [diff] [review]
9: some further tweaks

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

(In reply to Ms2ger from comment #52)
> lgtm, assuming Waldo is fine with the value thing.

Yeah, static fields beat static methods hands-down -- less cognitive overhead to them.  Had I been thinking of it at the time I'd probably have requested it, even.

::: mfbt/CheckedInt.h
@@ +273,5 @@
>  {
>      static bool run(U x)
>      {
> +      return x <= MaxValue<T>::value &&
> +             x >= MinValue<T>::value;

This probably fits on one line now.

@@ +292,5 @@
>  {
>      static bool run(U x)
>      {
> +      return sizeof(T) > sizeof(U) ||
> +             x <= U(MaxValue<T>::value);

Maybe this too?

@@ +384,5 @@
>  {
>      static bool run(T x, T y)
>      {
>        return y == 0 ||
> +             x <= MaxValue<T>::value / y;

And here.
Comment 55 Landry Breuil (:gaston) 2012-05-17 01:13:28 PDT
It seems patch 8 (http://hg.mozilla.org/mozilla-central/rev/d77e3cf5a162) broke the build on OpenBSD with a subtle failure :

content/media/nsBuiltinDecoderStateMachine.cpp:1130: error: no matching function for call to 'NS_MIN(PRInt64, long long int)'

see http://buildbot.rhaalovely.net/builders/mozilla-central-amd64/builds/390/steps/build/logs/stdio

I've already seen some similar failures in the past, PRInt64 is not long long int on OpenBSD. Since patch 1 switches CheckedInt to intX_t, i'd say changing the static_cast to int64_t might fix the fallout. Testing that.
Comment 56 Landry Breuil (:gaston) 2012-05-17 01:57:03 PDT
See bug 634793 for an explanation of PRInt64 vs int64_t / long long int on OpenBSD
Comment 57 Landry Breuil (:gaston) 2012-05-17 03:27:11 PDT
Created attachment 624691 [details] [diff] [review]
use stdint.h types instead of PRtypes, fixes build on OpenBSD

That patch fixes the build for me.
Comment 58 :Ms2ger (⌚ UTC+1/+2) 2012-05-17 03:37:33 PDT
Comment on attachment 624691 [details] [diff] [review]
use stdint.h types instead of PRtypes, fixes build on OpenBSD

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

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +1130,1 @@
>                               missingFrames.value());

Make it NS_MIN<int64_t>(UINT32_MAX, missingFrames.value()); please.
Comment 59 Landry Breuil (:gaston) 2012-05-17 05:49:52 PDT
Created attachment 624714 [details] [diff] [review]
use stdint.h types instead of PRtypes, fixes build on OpenBSD

Sure, that also works.
Comment 60 :Ms2ger (⌚ UTC+1/+2) 2012-05-17 05:50:44 PDT
Comment on attachment 624714 [details] [diff] [review]
use stdint.h types instead of PRtypes, fixes build on OpenBSD

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

Thanks
Comment 61 Landry Breuil (:gaston) 2012-05-17 06:38:23 PDT
Setting checkin-needed for att 624714
Comment 62 Benoit Jacob [:bjacob] (mostly away) 2012-05-17 06:39:44 PDT
I'll land both remaining patches in a moment.
Comment 63 Benoit Jacob [:bjacob] (mostly away) 2012-05-17 06:41:20 PDT
Btw, can't you get level 3 ? You have 60 patches in m-c already!
Comment 65 Landry Breuil (:gaston) 2012-05-17 07:43:28 PDT
(In reply to Benoit Jacob [:bjacob] from comment #63)
> Btw, can't you get level 3 ? You have 60 patches in m-c already!

Well.. you're the second one telling me this, but i'm not sure having "only" 60 patches warrants that :) I'll consider it.
Comment 66 Dão Gottwald [:dao] 2012-05-17 07:49:52 PDT
e:\builds\moz2_slave\m-in-w32\build\obj-firefox\dist\include\mozilla/CheckedInt.h(224) : fatal error C1004: unexpected end-of-file found
make[6]: *** [TestCheckedInt.obj] Error 2
make[5]: *** [libs] Error 2
make[4]: *** [libs_tier_base] Error 2
make[3]: *** [tier_base] Error 2
make[2]: *** [default] Error 2
make[1]: *** [realbuild] Error 2
make: *** [build] Error 2

backed out:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7bab95275e89
Comment 67 Benoit Jacob [:bjacob] (mostly away) 2012-05-17 07:50:42 PDT
There's a wiki page somewhere detailing the requirements, but I think that the requirement is more like 15 patches or so. Patch count is rather meaningless but at 60 patches, you should be good to go.
Comment 68 Benoit Jacob [:bjacob] (mostly away) 2012-05-17 08:03:58 PDT
MSVC is not actually a C++ compiler:

This code:

    static const IntegerType value =
        IsSigned<IntegerType>::value
        ? IntegerType(typename UnsignedType<IntegerType>::Type(1)
                      << PositionOfSignBit<IntegerType>::value)
        : IntegerType(0);

Gives these MSVC errors:

e:\builds\moz2_slave\m-in-w32\build\obj-firefox\dist\include\mozilla/CheckedInt.h(221) : error C2144: syntax error : 'int' should be preceded by ')'

        e:\builds\moz2_slave\m-in-w32\build\obj-firefox\dist\include\mozilla/CheckedInt.h(232) : see reference to class template instantiation 'mozilla::detail::MinValue<IntegerType>' being compiled

        with

        [

            IntegerType=uint8_t

        ]

        e:/builds/moz2_slave/m-in-w32/build/mfbt/tests/TestCheckedInt.cpp(406) : see reference to class template instantiation 'mozilla::detail::MaxValue<IntegerType>' being compiled

        with

        [

            IntegerType=uint8_t

        ]

        e:/builds/moz2_slave/m-in-w32/build/mfbt/tests/TestCheckedInt.cpp(461) : see reference to function template instantiation 'void test<int8_t>(void)' being compiled

e:\builds\moz2_slave\m-in-w32\build\obj-firefox\dist\include\mozilla/CheckedInt.h(222) : error C2059: syntax error : ')'

e:\builds\moz2_slave\m-in-w32\build\obj-firefox\dist\include\mozilla/CheckedInt.h(223) : error C2334: unexpected token(s) preceding ':'; skipping apparent function body

e:\builds\moz2_slave\m-in-w32\build\obj-firefox\dist\include\mozilla/CheckedInt.h(223) : error C2760: syntax error : expected '{' not ';'

e:\builds\moz2_slave\m-in-w32\build\obj-firefox\dist\include\mozilla/CheckedInt.h(224) : fatal error C1004: unexpected end-of-file found
Comment 69 Benoit Jacob [:bjacob] (mostly away) 2012-05-17 11:13:07 PDT
New try on windows: https://tbpl.mozilla.org/?tree=Try&rev=350a80c929f9
Comment 72 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-19 15:55:09 PDT
*** Bug 685775 has been marked as a duplicate of this bug. ***

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