Allow casting CheckedInts to other CheckedInt types

RESOLVED FIXED in mozilla24

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 761235 [details] [diff] [review]
patch: Add support for signed/unsigned conversion for CheckedInts

One thing that happens fairly often in code we want to be safe is interactions between signed and unsigned ints. Sometime, we want to take a signed CheckedInt which should be positive, and cast it to unsigned, and rely on its validity to tell us if we messed up.
Attachment #761235 - Flags: review?(jwalden+bmo)
Attachment #761235 - Flags: feedback?(bjacob)
Comment on attachment 761235 [details] [diff] [review]
patch: Add support for signed/unsigned conversion for CheckedInts

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

Watch out for trailing whitespace.

Also, this patch seems to allow generic construction from other CheckedInt types, which I agree is a good idea, but then the title of this bug needs updating.

::: mfbt/CheckedInt.h
@@ +619,5 @@
>                          "This type is not supported by CheckedInt");
>      }
> +    
> +    template<typename U>
> +    friend class CheckedInt;

A class template befriending itself?

Note that all specializations of a class template are "the same class" from the perspective of access control (public/private).

@@ +625,5 @@
> +    template<typename U>
> +    CheckedInt(CheckedInt<U> checked)
> +      : mValue(T(checked.mValue)),
> +        mIsValid(checked.mIsValid && detail::IsInRange<T>(checked.mValue))
> +    {

Copy the MOZ_STATIC_ASSERT guarding against unsupported types here.

@@ +626,5 @@
> +    CheckedInt(CheckedInt<U> checked)
> +      : mValue(T(checked.mValue)),
> +        mIsValid(checked.mIsValid && detail::IsInRange<T>(checked.mValue))
> +    {
> +    }

Also, doesn't this allow you to remove the above copy constructor? As this is a more generic version of it. C++ question for jwalden I guess.

::: mfbt/tests/TestCheckedInt.cpp
@@ +443,2 @@
>    }
> +  #define VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2(U) \

I'd swap the names of these macros so that the '2' one would be the one taking 2 parameters.

@@ +443,4 @@
>    }
> +  #define VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2(U) \
> +    VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(U,U) \
> +    VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(U,CheckedInt<U>)

That's all? So the unit test never checks construction of a CheckedInt from a different CheckedInt type?
Attachment #761235 - Flags: feedback?(bjacob) → feedback+
(Assignee)

Comment 2

5 years ago
(In reply to Benoit Jacob [:bjacob] from comment #1)
> Comment on attachment 761235 [details] [diff] [review]
> patch: Add support for signed/unsigned conversion for CheckedInts
> 
> Review of attachment 761235 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Watch out for trailing whitespace.
> 
> Also, this patch seems to allow generic construction from other CheckedInt
> types, which I agree is a good idea, but then the title of this bug needs
> updating.
> 
> ::: mfbt/CheckedInt.h
> @@ +619,5 @@
> >                          "This type is not supported by CheckedInt");
> >      }
> > +    
> > +    template<typename U>
> > +    friend class CheckedInt;
> 
> A class template befriending itself?
> 
> Note that all specializations of a class template are "the same class" from
> the perspective of access control (public/private).
This does not appear to be true. This is what I get if I remove that line:
 0:01.33 In file included from /home/jgilbert/dev/mozilla/central/mfbt/tests/TestCheckedInt.cpp:6:
 0:01.33 ../../dist/include/mozilla/CheckedInt.h:635:26: error: 'mValue' is a protected member of 'mozilla::CheckedInt<unsigned char>'
 0:01.33       : mValue(T(checked.mValue)),
 0:01.33                          ^
 0:01.33 /home/jgilbert/dev/mozilla/central/mfbt/tests/TestCheckedInt.cpp:449:3: note: in instantiation of function template specialization 'mozilla::CheckedInt<signed char>::CheckedInt<unsigned char>' requested here
 0:01.33   VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2(uint8_t)
 0:01.33   ^
 0:01.33 /home/jgilbert/dev/mozilla/central/mfbt/tests/TestCheckedInt.cpp:446:5: note: expanded from macro 'VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2'
 0:01.33     VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(U,CheckedInt<U>)
 0:01.33     ^
 0:01.33 /home/jgilbert/dev/mozilla/central/mfbt/tests/TestCheckedInt.cpp:430:21: note: expanded from macro 'VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE'
 0:01.33     VERIFY_IS_VALID(CheckedInt<T>(V(0))); \
 0:01.33                     ^
 0:01.33 /home/jgilbert/dev/mozilla/central/mfbt/tests/TestCheckedInt.cpp:49:43: note: expanded from macro 'VERIFY_IS_VALID'
 0:01.33 #define VERIFY_IS_VALID(x)   VERIFY_IMPL((x).isValid(), true)
 0:01.33                                           ^
 0:01.33 /home/jgilbert/dev/mozilla/central/mfbt/tests/TestCheckedInt.cpp:40:25: note: expanded from macro 'VERIFY_IMPL'
 0:01.33     verifyImplFunction((x), \
 0:01.33                         ^
 0:01.33 /home/jgilbert/dev/mozilla/central/mfbt/tests/TestCheckedInt.cpp:510:3: note: in instantiation of function template specialization 'test<signed char>' requested here
 0:01.33   test<int8_t>();
 0:01.33   ^
 0:01.33 ../../dist/include/mozilla/CheckedInt.h:598:7: note: declared protected here
 0:01.33     T mValue;
 0:01.33       ^


> 
> @@ +625,5 @@
> > +    template<typename U>
> > +    CheckedInt(CheckedInt<U> checked)
> > +      : mValue(T(checked.mValue)),
> > +        mIsValid(checked.mIsValid && detail::IsInRange<T>(checked.mValue))
> > +    {
> 
> Copy the MOZ_STATIC_ASSERT guarding against unsupported types here.
How would an unsupported type be created to be passed into this constructor, as it has to go through one of the more basic constructors at some point. The assert would literally say 'this CheckedInt you're giving us isn't a valid specialization of CheckedInt'.
> 
> @@ +626,5 @@
> > +    CheckedInt(CheckedInt<U> checked)
> > +      : mValue(T(checked.mValue)),
> > +        mIsValid(checked.mIsValid && detail::IsInRange<T>(checked.mValue))
> > +    {
> > +    }
> 
> Also, doesn't this allow you to remove the above copy constructor? As this
> is a more generic version of it. C++ question for jwalden I guess.
The above isn't really a copy constructor, though, is it? It wouldn't compile if it tried to instantiate with T=CheckedInt<foo>. It appears that the compiler is then creepily falling back to the default copy constructor.
> 
> ::: mfbt/tests/TestCheckedInt.cpp
> @@ +443,2 @@
> >    }
> > +  #define VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2(U) \
> 
> I'd swap the names of these macros so that the '2' one would be the one
> taking 2 parameters.
Done.
> 
> @@ +443,4 @@
> >    }
> > +  #define VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2(U) \
> > +    VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(U,U) \
> > +    VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(U,CheckedInt<U>)
> 
> That's all? So the unit test never checks construction of a CheckedInt from
> a different CheckedInt type?
No, that's exactly what this should be doing. We need to track both the underlying int type `U` as well as the type to construct-then-convert-from `V`. Notice how all tests and queries are against `U`, but when we construct based on a value, we construct a type `V`.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 3

5 years ago
Created attachment 761798 [details] [diff] [review]
patch
Assignee: nobody → jgilbert
Attachment #761235 - Attachment is obsolete: true
Attachment #761235 - Flags: review?(jwalden+bmo)
Attachment #761798 - Flags: review?(jwalden+bmo)
Attachment #761798 - Flags: review?(bjacob)
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 4

5 years ago
Created attachment 761823 [details] [diff] [review]
patch: Allow creating CheckedInts from other CheckedInts of differing sign and/or size.

Since our pseudo-copy constructor is templated, it doesn't count as an actual copy constructor, so the compiler generates one for us.

I talked with Waldo and I think it's worth expanding this to convert between sizes (with checks!), as well as signedness. However, I noticed that templating the constructor allows for accidentally casting between types on assignment, like this:
CheckedInt<A> foo(x);
CheckedInt<B> bar(y);
...
foo = bar + 1;

Instead, how about a templated Cast<U>() member function?
Attachment #761798 - Attachment is obsolete: true
Attachment #761798 - Flags: review?(jwalden+bmo)
Attachment #761798 - Flags: review?(bjacob)
Attachment #761823 - Flags: review?(jwalden+bmo)
Attachment #761823 - Flags: review?(bjacob)
Flags: needinfo?(jwalden+bmo)

Comment 5

5 years ago
Comment on attachment 761823 [details] [diff] [review]
patch: Allow creating CheckedInts from other CheckedInts of differing sign and/or size.

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

Looks good, basically nitpicks except for the "Cast" naming bit.

::: mfbt/CheckedInt.h
@@ +631,5 @@
>      }
> +    
> +    template<typename U>
> +    friend class CheckedInt;
> +    

Trailing whitespace, twice.

@@ +633,5 @@
> +    template<typename U>
> +    friend class CheckedInt;
> +    
> +    template<typename U>
> +    CheckedInt<U> Cast() const

mfbt method naming style is camelCaps.  But "cast" is a bit terse.  Maybe toChecked?  Then you'd see

   Checked<uint16_t> u = UINT16_MAX;
   Checked<uint8_t> v = u.toChecked<uint8_t>();

which is clearer about what's being cast to.

@@ +637,5 @@
> +    CheckedInt<U> Cast() const
> +    {
> +        CheckedInt<U> ret(mValue);
> +        ret.mIsValid = ret.mIsValid && mIsValid;
> +        return ret;

Two-space indent, not four.

::: mfbt/tests/TestCheckedInt.cpp
@@ +444,4 @@
>  
> +  /* Check that construction of CheckedInt from an integer value of a mismatched type is checked
> +   * Also check casting between all types.
> +   */

Either use // commenting -- looks consistent with above -- or this should be

/*
 * comment line 1...
 * comment line 2...
 */

@@ +445,5 @@
> +  /* Check that construction of CheckedInt from an integer value of a mismatched type is checked
> +   * Also check casting between all types.
> +   */
> +
> +  #define VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2(U,V,X) \

Name "X" something like AfterExpr or something.

@@ +465,3 @@
>    }
> +  #define VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(U) \
> +    VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2(U,U,) \

Empty macro parameters are only valid in C99/C++11.  It looks like this file's compiled as C++11 at least on my system, but you should probably be aware of this in case there are complications I'm not anticipating.

@@ +465,5 @@
>    }
> +  #define VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(U) \
> +    VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2(U,U,) \
> +    VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2(U,CheckedInt<U>,.Cast<T>())
> +  

Trailing whitespace.

@@ +496,5 @@
>    VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(longLong)
>    VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(unsignedLongLong)
>  
>    /* Test increment/decrement operators */
> +  

Trailing whitespace.
Attachment #761823 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 761823 [details] [diff] [review]
patch: Allow creating CheckedInts from other CheckedInts of differing sign and/or size.

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

::: mfbt/CheckedInt.h
@@ +636,5 @@
> +    template<typename U>
> +    CheckedInt<U> Cast() const
> +    {
> +        CheckedInt<U> ret(mValue);
> +        ret.mIsValid = ret.mIsValid && mIsValid;

Use &&= ?
Attachment #761823 - Flags: review?(bjacob) → review+
Summary: Allow constructing CheckedInts from same-size,different-signedness CheckedInts. → Allow casting CheckedInts to other CheckedInt types
(Assignee)

Comment 7

5 years ago
Created attachment 762952 [details] [diff] [review]
patch with nits
Attachment #761823 - Attachment is obsolete: true
Attachment #762952 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9e0bf0548e48
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.