Closed Bug 874764 Opened 7 years ago Closed 7 years ago

CheckedInt types should support operator%.

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(1 file, 4 obsolete files)

operator% is not hard to make safe, but it is a commonly used operation in otherwise-unsafe code, for alignment padding.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jgilbert
Attachment #752537 - Flags: review?(bjacob)
Comment on attachment 752537 [details] [diff] [review]
patch

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

As discussed offline, this patch needs a little more work (to compile).
Attachment #752537 - Flags: review?(bjacob) → review-
Duplicate of this bug: 881979
Attachment #752537 - Attachment is obsolete: true
Attachment #761239 - Flags: review?(bjacob)
Comment on attachment 761239 [details] [diff] [review]
patch: add support for modulus to CheckedInt

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

I reviewed this in the dup, before you could mark the dup, might as well copy over the comments.  :-)  Looks fine, seems good to have the second set of eyes on this so it's properly checked (pun intended).

::: mfbt/CheckedInt.h
@@ +180,5 @@
>                                                      false>::Type Type;
>  };
>  
>  template<typename IntegerType>
> +struct SignedType

I'd bet {S,Uns}ignedType could be replaced by mozilla::Make{S,Uns}igned, but WebKit convenience, sigh, ish.  Something to noodle over some other time.

@@ +472,5 @@
> +   */
> +  if (IsSigned<T>::value) {
> +    typename SignedType<T>::Type sx = x;
> +    if (sx < 0)
> +      return false;

Wouldn't surprise me if this triggers compiler warnings somewhere, but we can throw more templates at it if needed later.  (MOAR TEMPLATES)
Attachment #761239 - Flags: review+
Comment on attachment 761239 [details] [diff] [review]
patch: add support for modulus to CheckedInt

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

::: mfbt/CheckedInt.h
@@ +463,5 @@
> +{
> +  /*
> +   * Mod is pretty trivial.
> +   * If x or y are negative, the results are implementation defined.
> +   *   Consider these invalid.

This is true in ANSI C, but not C99, C11, C++98, or C++11.

From C++11:
The binary / operator yields the quotient, and the binary % operator yields the remainder from the division
of the first expression by the second. If the second operand of / or % is zero the behavior is undefined. For
integral operands the / operator yields the algebraic quotient with any fractional part discarded;80 if the
quotient a/b is representable in the type of the result, (a/b)*b + a%b is equal to a.

[tl;dr - a % b is well-defined unless b is 0 or a = INT_MIN and b = -1.]
I'd be inclined to adjust the comment, but still stick to the current patch semantics, for simplicity.  :-)  Unless more people have internalized these rules than I expect, but I kind of doubt it.
Comment on attachment 761239 [details] [diff] [review]
patch: add support for modulus to CheckedInt

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

So the patch is fine as Jeff says, but at the same time, we can point out a better way to implement the branching on signedness, so let's flag this r- to ensure that this doesn't get forgotten.

::: mfbt/CheckedInt.h
@@ +472,5 @@
> +   */
> +  if (IsSigned<T>::value) {
> +    typename SignedType<T>::Type sx = x;
> +    if (sx < 0)
> +      return false;

So to pile on existing review comments and add one: the current approach requires you to introduce SignedType<T> above (or rely on other MFBT helpers) and as Jeff says, is quite likely to cause compiler warnings (specifically on MSVC). In addition, it is a bit counter-intuitive to cast x to SignedType<T> inside an if() block in which we know for sure that T is signed already.

So how about addressing all these concerns at once by moving the branching on signedness to compile time with a template helper?
Attachment #761239 - Flags: review?(bjacob) → review-
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> I'd bet {S,Uns}ignedType could be replaced by mozilla::Make{S,Uns}igned, but
> WebKit convenience, sigh, ish.

If it were just about WebKit convenience, I'd say that it's OK to depend on other MFBT headers. I see it more as the generic convenience of having somewhere a single-header implementation of checked integer arithmetic. That said, even that is negociable, and if we really needed something like SignedType<T> (which we don't, see my above review comment), it'd be reasonable to #include the MFBT header providing one.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> I'd be inclined to adjust the comment, but still stick to the current patch
> semantics, for simplicity.  :-)  Unless more people have internalized these
> rules than I expect, but I kind of doubt it.

I agree.
Attached patch patch (obsolete) — Splinter Review
Now with more partial template specialization fun!
Attachment #761239 - Attachment is obsolete: true
Attachment #761790 - Flags: review?(bjacob)
Attached patch patch, qref'd (obsolete) — Splinter Review
Forgot to qref...
Attachment #761790 - Attachment is obsolete: true
Attachment #761790 - Flags: review?(bjacob)
Attachment #761792 - Flags: review?(bjacob)
Comment on attachment 761792 [details] [diff] [review]
patch, qref'd

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

r+ with nits, I'm being very nitpicky here, but please address, it's about keeping the style of this code self-consistent and avoiding a warning that we once hit in this file.

::: mfbt/CheckedInt.h
@@ +449,5 @@
>    return y != 0 &&
>           !(IsSigned<T>::value && x == MinValue<T>::value && y == T(-1));
>  }
>  
>  template<typename T, bool IsSigned = IsSigned<T>::value>

I think that this template parameter needs to be renamed to IsTSigned, to prevent some issues (warnings, maybe) on some compilers (MSVC-something, maybe) complaining when it's the same identifier as the IsSigned template, as was done elsewhere in this file.

@@ +451,5 @@
>  }
>  
>  template<typename T, bool IsSigned = IsSigned<T>::value>
> +struct IsModValid2 {
> +  static inline bool Test(T x, T y);

Leave this unspecialized template empty, so that accidentally calling the generic case will give a simpler compilation error, and move the specializations above IsModValid.

@@ +474,5 @@
> + */
> +
> +template<typename T>
> +struct IsModValid2<T, false> {
> +  static inline bool Test(T x, T y) {

For consistency with what's done elsewhere in this file as well as in other MFBT and STL code, replace these Test functions by |static const bool value| constants. The name |value|, in particular, is really quite standard for these idioms, and so it helps people reading code.
Attachment #761792 - Flags: review?(bjacob) → review+
Also, please rename IsModValid2 to IsModValidImpl, again for the sake of idiom consistency.
Attached patch patch with nitsSplinter Review
With nits addressed.
Attachment #761792 - Attachment is obsolete: true
Attachment #762949 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a74b3d43c3af
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.