685775 - Move CheckedInt.h to mozilla/

Status: RESOLVED DUPLICATE of bug 732875

(Core :: MFBT, defect)

Description

[Joe Drew (not getting mail)]

Attachment: move CheckedInt.h to include/mozilla/

CheckedInt.h is currently just in the toplevel include directory, but because mozilla::CheckedInt is in the mozilla namespace, the include file should be in the mozilla/ directory.

One question - should CheckedInt.h be moved to mfbt? It has standalone unit tests in xpcom/tests.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Does js/src use or need something like

/** \class CheckedInt
  * \brief Integer wrapper class checking for integer overflow and other errors
  * \param T the integer type to wrap. Can be any of PRInt8, PRUint8, PRInt16, PRUint16,
  *          PRInt32, PRUint32, PRInt64, PRUint64.
  *
  * This class implements guarded integer arithmetic. Do a computation, check that
  * valid() returns true, you then have a guarantee that no problem, such as integer overflow,
  * happened during this computation.
  *
  * The arithmetic operators in this class are guaranteed not to crash your app
  * in case of a division by zero.
  *
  * For example, suppose that you want to implement a function that computes (x+y)/z,
  * that doesn't crash if z==0, and that reports on error (divide by zero or integer overflow).
  * You could code it as follows:
    \code
    PRBool compute_x_plus_y_over_z(PRInt32 x, PRInt32 y, PRInt32 z, PRInt32 *result)
    {
        CheckedInt<PRInt32> checked_result = (CheckedInt<PRInt32>(x) + y) / z;
        *result = checked_result.value();
        return checked_result.valid();
    }
    \endcode

?  If not, probably better to keep in xpcom land.

Comment 2

[Luke Wagner [:luke]]

Hm, potentially!  There are many places where we fuss with integer overflow although we try to keep most of it behind js::Vector and such.  I'm guessing it would take some work to un-PR-ify this, though?

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

I do plan to un-PR-ify it. For that matter, WebKit is already using an un-PR-ified version of it.

* replace PRBool by bool
* add ability to use standard integer types like int. This requires a 2nd pass of template specializations. We have a patch bitrotting around doing that.

Does JS have its own sized integer types or are you only interested in standard non-sized integer types like int?

Comment 4

[Luke Wagner [:luke]]

JS does care about sized int types.  On the subject, though, I'm not sure which types should be used in mfbt.  Do you know cjones?  I notice everything in mfbt so far has avoiding the problem via 'int' and 'size_t'.  There's a pretty sad story (bug 662852) of why neither int32_t nor int32 work public headers.  On the bright side, the issue is only with linker symbols; for an inlined template, int32_t seems like it would work.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

The problem is that CheckedInt must directly support all size integer types that we want to use it with. Otherwise, it would force its user to do type conversions on the caller side before using CheckedInt, and that would open a new door for bugs, defeating the point of CheckedInt.

The idea is that CheckedInt allows you to do

   CheckedInt<PRInt32> x = some_expression;

and it will work, and correctly check for integer overflow, no matter what is the type of some_expression. So it's very easy to review CheckedInt-using code and be convinced that it's bug-free.

So the easiest is to build into CheckedInt support for all sets of sized types  (PRInt*, int*_t, etc...) that it will be used with. This means that CheckedInt depends on all these types being defined. Currently, CheckedInt is defined only for PR types, and is defined in xpcom/ds where PR types are available.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Now we're moving toward stdint, should we pick this up again?

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Yes. Planning to come back to it this week.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 559369 [details] [diff] [review]
move CheckedInt.h to include/mozilla/

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

Sorry for the extremely late review. Here's what happened: I misunderstood this as moving CheckedInt to mfbt/ and thought: fine but there's a couple things that should happen first. And never got the time to do them.

Now, moving to mfbt/ is a high priority now (bug 731017) so let's do this as a precursor change.

Things that need to happen now before we can move to mfbt/:
 - use StdInt types instead of PRInt types
 - 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 9

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: rebased/updated

Comment 10

[Jeff Walden [:Waldo]]

I think this basically turned into a dup of bug 732875 in the meantime; reopen if I missed some nuance here.