Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
141583008:
Add support for safe math operations in base/numerics (Closed)
|
Created:
6 years, 11 months ago by jschuh Modified:
4 years, 11 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, Ryan Sleevi Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd support for safe math operations in base/numerics
Add checked numeric templates.
BUG=332611
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253789
Patch Set 1 #Patch Set 2 : #Patch Set 3 : WIP: Initial implementation #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 1
Patch Set 8 : msvs 2010 fixes #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : WIP: Fixed floating piont constructor #Patch Set 12 : Arithmetic tests and a float range fix #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : Documentation and re-added ValueFloating. #
Total comments: 21
Patch Set 19 : style fixes #
Total comments: 39
Patch Set 20 : WIP: Actioned comments #Patch Set 21 : Actioned feedback (really I swear) #
Total comments: 23
Patch Set 22 : Actioned feedback #
Total comments: 6
Patch Set 23 : Nits #Patch Set 24 : clang fix #Patch Set 25 : Fix x64 _mm_empty #
Total comments: 1
Messages
Total messages: 37 (1 generated)
https://codereview.chromium.org/141583008/diff/660001/base/numerics/safe_math... File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/141583008/diff/660001/base/numerics/safe_math... base/numerics/safe_math_impl.h:340: } So, templated CTORS = generally a bad/tricky thing to get right. On the upside, enable_if (C++11, though it can be emulated) to the rescue, along with a default value Look at https://codereview.chromium.org/11411318/diff/3001/base/memory/scoped_ptr.h for an example of where I want to allow a templated constructor, but only if a given type is a numeric/integral type (which, bee tee dubs, you can use std::is_integral rather than numeric_limits<T>::is_specialized, although it's more pedantry than anything) If I were to speculative rewrite lines 326-340, without testing (because who tests code before writing code reviews) template <typename Src> explicit CheckedNumericState(Src value, typename std::enable_if<std::numeric_limits<Src>::is_specialized, int>::type = 0); ^- the above creates a valid definition of a ctor with a second arg (and integer) that defaults to 0 for all types Src that are specialized with numeric limits. For non-specialized types, ::type is ambiguous, and thus SFINAE fails to match this ctor. You can then also repeat the trick with template <typename Src> explicit CheckedNumericState(const CheckedNumericState<Src>& rhs, typename std::enable_if<std::numeric_limits<Src>::is_specialized, int>::type = 0); However, I'm not sure if this second definition will be seen consistently when performing templated resolution, since really this second ctor is a specialization of the first. If it doesn't work, we can rewrite the first template and then specialize it
This is either ready to review or I'm ready to give up for now (probably some combination of both).
Math is hard! Can I go shopping? Note I just reviewed the C++ Template foo at this time - I have not dug into the actual correctness of the implementation or math. That will... take a bit, and I don't think I'll be able to get to it for a few days. https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_con... File base/numerics/safe_conversions_impl.h (right): https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:25: SRC_SIGNED = 1 Why use these enums versus a bool? https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:30: CONTAINS_RANGE Why no literals for these, compared with 19/20 and 24/25? https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:45: template <typename Dst, typename Src, We traditionally format template arguments as with function declarations, although I realize internal style has changed. Namely template <typename Dst, typename Src, DstSignId ... https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:63: StaticRangeCheck<Dst, Src, DST_SIGNED, SRC_SIGNED> {}; Doesn't this actually leave ::value unspecified? https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:69: typedef numeric_limits<Src> SrcLimits; Unused? https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... base/numerics/safe_math.h:17: // dat types, and contains overloads for basic arithmetic operations (i.e.: +, typo: data https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... base/numerics/safe_math.h:48: friend class CheckedNumeric; Does this actually work? I didn't think generic friends like this were supported in MSVC? https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... base/numerics/safe_math.h:55: state_(rhs.state_.value(), rhs.state_.validity()) {} git-cl-format this CheckedNumeric(const CheckedNumeric<Src>& rhs) : state_(....) {} https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... base/numerics/safe_math.h:75: RangeCheckId validity() const { return state_.validity(); } if it's only for tests, you can call it validity() [the internal method] and ValidityForTesting() [public method], the latter which will yak in presubmit.py if someone uses it outside of a test file. https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... base/numerics/safe_math.h:87: T ValueUnsafe() const { return state_.value(); } Ditto https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... base/numerics/safe_math_impl.h:62: size_t>::type value = 8 * sizeof(Integer) - 1; Does MSVC let you get away with this? I seem to recall it yaks if assigning a value to static storage in the decl. (absent constexpr)
https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_con... File base/numerics/safe_conversions_impl.h (right): https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:25: SRC_SIGNED = 1 On 2014/02/10 23:42:41, Ryan Sleevi wrote: > Why use these enums versus a bool? It makes the template instantiations easier to follow (and was suggested by earlier reviewers). https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:30: CONTAINS_RANGE On 2014/02/10 23:42:41, Ryan Sleevi wrote: > Why no literals for these, compared with 19/20 and 24/25? Forgot. https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:45: template <typename Dst, typename Src, On 2014/02/10 23:42:41, Ryan Sleevi wrote: > We traditionally format template arguments as with function declarations, > although I realize internal style has changed. > > Namely > > template <typename Dst, > typename Src, > DstSignId ... Done. https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:63: StaticRangeCheck<Dst, Src, DST_SIGNED, SRC_SIGNED> {}; On 2014/02/10 23:42:41, Ryan Sleevi wrote: > Doesn't this actually leave ::value unspecified? No, it's inheriting value from the DST_SIGNED, SRC_SIGNED version. https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:69: typedef numeric_limits<Src> SrcLimits; On 2014/02/10 23:42:41, Ryan Sleevi wrote: > Unused? Done. https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... base/numerics/safe_math.h:17: // dat types, and contains overloads for basic arithmetic operations (i.e.: +, On 2014/02/10 23:42:41, Ryan Sleevi wrote: > typo: data Done. https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... base/numerics/safe_math.h:48: friend class CheckedNumeric; On 2014/02/10 23:42:41, Ryan Sleevi wrote: > Does this actually work? I didn't think generic friends like this were supported > in MSVC? Yep, works correct in msvc, gcc, and clang. https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... base/numerics/safe_math.h:55: state_(rhs.state_.value(), rhs.state_.validity()) {} On 2014/02/10 23:42:41, Ryan Sleevi wrote: > git-cl-format this > > CheckedNumeric(const CheckedNumeric<Src>& rhs) > : state_(....) {} Done. https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... base/numerics/safe_math.h:75: RangeCheckId validity() const { return state_.validity(); } On 2014/02/10 23:42:41, Ryan Sleevi wrote: > if it's only for tests, you can call it validity() [the internal method] and > ValidityForTesting() [public method], the latter which will yak in presubmit.py > if someone uses it outside of a test file. So... that doesn't solve the nasty mess of friend overloads I'd. Also, I'm expecting some non-negligible work of foisting math into CheckedNumericState when I have to approach the issue of consistent saturation semantics. So, for now I'd much rather punt on this. https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/141583008/diff/1400001/base/numerics/safe_mat... base/numerics/safe_math_impl.h:62: size_t>::type value = 8 * sizeof(Integer) - 1; On 2014/02/10 23:42:41, Ryan Sleevi wrote: > Does MSVC let you get away with this? I seem to recall it yaks if assigning a > value to static storage in the decl. (absent constexpr) Static consts have been supported for quite a while. The trybots do not lie (much).
Still looking through, but here are initial comments. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... File base/numerics/safe_conversions_impl.h (right): https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:18: enum DstSignId { DST_UNSIGNED = 0, DST_SIGNED = 1 }; (push back on this if you disagree) DstSignId and SrcSignId seems like it should be collapsed into one: enum IntegerRepresentation { INTEGER_REPRESENTATION_SIGNED, INTEGER_REPRESENTATION_UNSIGNED, }; or some sensible shortening of that. I don't see too much benefit to differnt types. DstRepr and SrcRepr look better as type-variable names rather than type names. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:22: enum DstRangeId { OVERLAPS_RANGE = 0, CONTAINS_RANGE = 1 }; nit: enum DstRange { DST_RANGE_OVERLAPS, DST_RANGE_CONTAINS, }; Also, can you add a comment explaining what exactly it means to be overlapping and containing? Are we talking integers? floats? both? Open intervals? Closed? Half-open? Also, maybe move the enum closer to where you actually use it? https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:24: // Retrieve the max exponent for floating types and compute it for integrals. In the comment, prefer explaining why the math is sensible rather than what the function is doing. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:25: template <typename NumericType> s/NumericType/IntegralType/ Also, is there an easy COMPILE_ASSERT we can do to ensure this is an integral type? If not, don't worry...just curious. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:42: : SRC_UNSIGNED > struct StaticRangeCheck {}; Put the "struct" on a new line. As is, I spent 10 mins wondering how you managed a "greater than" comparison with a type declaration. Also, can you just make this a declaration instead of a struct definition? I don't see a reason to provide a default template. Only the specializations provided below seem to make semantic sense. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:60: // Unsigned to signed, overlapping. These comments in general don't explain why the relations are correct. Please update here and below. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:62: struct StaticRangeCheck<Dst, Src, DST_SIGNED, SRC_UNSIGNED> { Maybe called this DstRangeRelationToSrcRange or something that implies that you mean dst contains src, and not src contains dest. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:64: MaxExponent<Dst>::value > MaxExponent<Src>::value ? CONTAINS_RANGE >= doesn't constitute contains? https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:74: enum RangeCheckId { nit: "Id" is a weird suffix for enums here. These aren't really identifiers. I would call it something like RangeConstraints or something. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:84: #define BASE_NUMERIC_RANGE_CHECK_RESULT(is_in_upper_bound, is_in_lower_bound) \ Why is this a macro instead of a static inline function? static inline RangeCheckId GetRangeType(bool is_in_upper_bound, bool is_in_lower_bound) { ((is_in_upper_bound) ? 0 : TYPE_OVERFLOW) ((is_in_lower_bound) ? 0 : TYPE_UNDERFLOW) }; https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:95: StaticRangeCheck<Dst, Src>::value > struct RangeCheckImpl {}; Don't define this struct. Only declare it. That way you get a whitelist of valid specializations. Invoking the "default" specialization doesn't seem semantically valid. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math.h:195: #define BASE_NUMERIC_ARITHMETIC_OPERATORS( \ ?! Is there some way to make this more sane? I feel like you could use a template that took a function object and then pass in a bunch of those similar to how less<T> in STL is implemented.
more comments. You should probably find me over chat. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math.h:38: // if (checked_size.IsValid() && checked_size.ValueOrDie() < buffer_size) I don't see where checked_int and checked_size are coming from. Can you show a declaration too in this example? https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math.h:108: T ValueFloating() { Why isn't this const? https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math.h:114: template <typename Src> Ugh. Do you really need overloaded operators? https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math_impl.h:29: struct IntegerForSizeAndSign {}; Here and elsewhere, don't provide a definition for the template declaration if it doesn't have semantics that make sense. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math_impl.h:62: // The ArithmeticPromotion template below will need to be updated (or more This looks like a TODO. Make it into a TODO? Also add a newline before the comment so it's visually separated from the specialization blob. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math_impl.h:67: typedef typename enable_if< Why do we need an enable_if here? Normally you use enable_ifs to remove overloads that would become ambiguous when a template is instantiated with a certain combinations of types. There doesn't seem to be any overloads here. What compile error are you protecting again (or trying to ensure)? https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math_impl.h:174: typename enable_if<numeric_limits<T>::is_integer&& numeric_limits< Yes...it's looking like you're using enable_if to force the template to not be instantiable if the type is incorrect. This seems better suited to a COMPILE_ASSERT().
https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math.h:195: #define BASE_NUMERIC_ARITHMETIC_OPERATORS( \ On 2014/02/11 19:40:10, awong wrote: > ?! > > Is there some way to make this more sane? I feel like you could use a template > that took a function object and then pass in a bunch of those similar to how > less<T> in STL is implemented. To expand on this, assume you wrote a bunch of silly wrappers like template <typename T> struct AddOp { T operator()(T a, T b) { return a+b; } }; Or even, if you want to get more crazy: template <typename T> struct AddOp { static T Evaluate(T a, T b) { return a+b; } static T CheckedEvaluate(T a, T b, RangeCheck* check) { /* code */ } }; Then you could rewrite this as template <typename T, typename Op> struct GenericOp { T EvaulateOp(T lhs, T rhs) { return Op::Evluate(lhs, rhs); }; T& EvaulateCompoundOp(T& lhs, T rhs) { lhs = Op::Evluate(lhs, rhs); return lhs; }; } template <typename T> CheckedNumeric<typename ArithmeticPromotion<T>::type> operator+( const CheckedNumeric<T>& lhs, const CheckedNumeric<T>& rhs) { return GenericOp<T, AddOp>(lhs, rhs); }; or something similar?
On 2014/02/11 21:02:11, awong wrote: > https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_math.h > File base/numerics/safe_math.h (right): > > https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... > base/numerics/safe_math.h:195: #define BASE_NUMERIC_ARITHMETIC_OPERATORS( > \ > On 2014/02/11 19:40:10, awong wrote: > > ?! > > > > Is there some way to make this more sane? I feel like you could use a template > > that took a function object and then pass in a bunch of those similar to how > > less<T> in STL is implemented. > > > To expand on this, assume you wrote a bunch of silly wrappers like > > template <typename T> struct AddOp { > T operator()(T a, T b) { return a+b; } > }; > > Or even, if you want to get more crazy: > template <typename T> struct AddOp { > static T Evaluate(T a, T b) { return a+b; } > static T CheckedEvaluate(T a, T b, RangeCheck* check) { /* code */ } > }; > > Then you could rewrite this as > > template <typename T, typename Op> > struct GenericOp { > T EvaulateOp(T lhs, T rhs) { > return Op::Evluate(lhs, rhs); > }; > T& EvaulateCompoundOp(T& lhs, T rhs) { > lhs = Op::Evluate(lhs, rhs); > return lhs; > }; > } > > template <typename T> > CheckedNumeric<typename ArithmeticPromotion<T>::type> operator+( > const CheckedNumeric<T>& lhs, const CheckedNumeric<T>& rhs) { > return GenericOp<T, AddOp>(lhs, rhs); > }; > > > or something similar? If you're clever, you can even avoid the T in GenericOp.
See https://code.google.com/p/chromium/issues/detail?id=332611#c3 and #4 for details on failures seen running on VS2013.
I think we're good to go now. I've added TODOs for the things I will be iterating on based on our discussions. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... File base/numerics/safe_conversions_impl.h (right): https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:18: enum DstSignId { DST_UNSIGNED = 0, DST_SIGNED = 1 }; On 2014/02/11 19:40:10, awong wrote: > (push back on this if you disagree) > > DstSignId and SrcSignId seems like it should be collapsed into one: > > enum IntegerRepresentation { > INTEGER_REPRESENTATION_SIGNED, > INTEGER_REPRESENTATION_UNSIGNED, > }; > > or some sensible shortening of that. > > I don't see too much benefit to differnt types. DstRepr and SrcRepr look better > as type-variable names rather than type names. > Done. Fred actually wanted these split, because he thought it made the specializations easier to follow. Although, collapsing them allows me to eliminate one specialization (which I prefer), so I've collapsed them again and will rely on the comments for additional clarity. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:22: enum DstRangeId { OVERLAPS_RANGE = 0, CONTAINS_RANGE = 1 }; On 2014/02/11 19:40:10, awong wrote: > nit: > > enum DstRange { > DST_RANGE_OVERLAPS, > DST_RANGE_CONTAINS, > }; > > Also, can you add a comment explaining what exactly it means to be overlapping > and containing? Are we talking integers? floats? both? Open intervals? Closed? > Half-open? > > Also, maybe move the enum closer to where you actually use it? Done. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:24: // Retrieve the max exponent for floating types and compute it for integrals. On 2014/02/11 19:40:10, awong wrote: > In the comment, prefer explaining why the math is sensible rather than what the > function is doing. Done. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:25: template <typename NumericType> On 2014/02/11 19:40:10, awong wrote: > s/NumericType/IntegralType/ NumericType is correct because it handles both floating point and integral (but floating point is just a pass-thru. > Also, is there an easy COMPILE_ASSERT we can do to ensure this is an integral > type? If not, don't worry...just curious. I could use an enable_if, if you want me to add that in later. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:42: : SRC_UNSIGNED > struct StaticRangeCheck {}; On 2014/02/11 19:40:10, awong wrote: > Put the "struct" on a new line. As is, I spent 10 mins wondering how you managed > a "greater than" comparison with a type declaration. > > Also, can you just make this a declaration instead of a struct definition? I > don't see a reason to provide a default template. Only the specializations > provided below seem to make semantic sense. Done (but you can blame "git cl format" for that one :). https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:60: // Unsigned to signed, overlapping. On 2014/02/11 19:40:10, awong wrote: > These comments in general don't explain why the relations are correct. Please > update here and below. Done. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:62: struct StaticRangeCheck<Dst, Src, DST_SIGNED, SRC_UNSIGNED> { On 2014/02/11 19:40:10, awong wrote: > Maybe called this DstRangeRelationToSrcRange > > or something that implies that you mean > dst contains src, and not src contains dest. Done. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:64: MaxExponent<Dst>::value > MaxExponent<Src>::value ? CONTAINS_RANGE On 2014/02/11 19:40:10, awong wrote: > >= doesn't constitute contains? Done. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:74: enum RangeCheckId { On 2014/02/11 19:40:10, awong wrote: > nit: "Id" is a weird suffix for enums here. These aren't really identifiers. I > would call it something like RangeConstraints or something. Done. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:84: #define BASE_NUMERIC_RANGE_CHECK_RESULT(is_in_upper_bound, is_in_lower_bound) \ On 2014/02/11 19:40:10, awong wrote: > Why is this a macro instead of a static inline function? > > > static inline RangeCheckId GetRangeType(bool is_in_upper_bound, bool > is_in_lower_bound) { > ((is_in_upper_bound) ? 0 : TYPE_OVERFLOW) > ((is_in_lower_bound) ? 0 : TYPE_UNDERFLOW) > }; Done. This needed to be a macro several versions back, but you're right that it can just be an inline now. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:95: StaticRangeCheck<Dst, Src>::value > struct RangeCheckImpl {}; On 2014/02/11 19:40:10, awong wrote: > Don't define this struct. Only declare it. > > That way you get a whitelist of valid specializations. > > Invoking the "default" specialization doesn't seem semantically valid. Done. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math.h:38: // if (checked_size.IsValid() && checked_size.ValueOrDie() < buffer_size) On 2014/02/11 20:14:12, awong wrote: > I don't see where checked_int and checked_size are coming from. Can you show a > declaration too in this example? Done. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math.h:108: T ValueFloating() { On 2014/02/11 20:14:12, awong wrote: > Why isn't this const? Done. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math.h:114: template <typename Src> On 2014/02/11 20:14:12, awong wrote: > Ugh. Do you really need overloaded operators? Well, if I want it to be easy to use. :) https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math.h:195: #define BASE_NUMERIC_ARITHMETIC_OPERATORS( \ On 2014/02/11 21:02:11, awong wrote: > On 2014/02/11 19:40:10, awong wrote: > > ?! > > > > Is there some way to make this more sane? I feel like you could use a template > > that took a function object and then pass in a bunch of those similar to how > > less<T> in STL is implemented. > > > To expand on this, assume you wrote a bunch of silly wrappers like > > template <typename T> struct AddOp { > T operator()(T a, T b) { return a+b; } > }; > > Or even, if you want to get more crazy: > template <typename T> struct AddOp { > static T Evaluate(T a, T b) { return a+b; } > static T CheckedEvaluate(T a, T b, RangeCheck* check) { /* code */ } > }; > > Then you could rewrite this as > > template <typename T, typename Op> > struct GenericOp { > T EvaulateOp(T lhs, T rhs) { > return Op::Evluate(lhs, rhs); > }; > T& EvaulateCompoundOp(T& lhs, T rhs) { > lhs = Op::Evluate(lhs, rhs); > return lhs; > }; > } > > template <typename T> > CheckedNumeric<typename ArithmeticPromotion<T>::type> operator+( > const CheckedNumeric<T>& lhs, const CheckedNumeric<T>& rhs) { > return GenericOp<T, AddOp>(lhs, rhs); > }; > > > or something similar? Punting for now based on our conversation, but I will de-macrofy this in a follow-up. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math_impl.h:29: struct IntegerForSizeAndSign {}; On 2014/02/11 20:14:12, awong wrote: > Here and elsewhere, don't provide a definition for the template declaration if > it doesn't have semantics that make sense. Done. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math_impl.h:62: // The ArithmeticPromotion template below will need to be updated (or more On 2014/02/11 20:14:12, awong wrote: > This looks like a TODO. Make it into a TODO? > > Also add a newline before the comment so it's visually separated from the > specialization blob. We don't support 128-bit math today, so it's a warning. However, I will make it explicitly such. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math_impl.h:67: typedef typename enable_if< On 2014/02/11 20:14:12, awong wrote: > Why do we need an enable_if here? Normally you use enable_ifs to remove > overloads that would become ambiguous when a template is instantiated with a > certain combinations of types. There doesn't seem to be any overloads here. > > What compile error are you protecting again (or trying to ensure)? I'm preventing this: UnsignedIntegerForSize<float>::type That would give you an uint32_t, which is very wrong. https://codereview.chromium.org/141583008/diff/1490001/base/numerics/safe_mat... base/numerics/safe_math_impl.h:174: typename enable_if<numeric_limits<T>::is_integer&& numeric_limits< On 2014/02/11 20:14:12, awong wrote: > Yes...it's looking like you're using enable_if to force the template to not be > instantiable if the type is incorrect. > > This seems better suited to a COMPILE_ASSERT(). This is breaking out the specializations. Based on our discussion, I added a TODO and will fix this when I coalesce the math primitives into CheckedNumericState (which I've determined I need to do in order to easily support integer saturation semantics).
next pass of comments. getting pretty close. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... File base/numerics/safe_conversions_impl.h (right): https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:27: enum IntegerSignRepresentation { s/IntegerSignRepresentation/IntegerRepresentation/g https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:55: struct StaticDstRangeRelationToSrcRange; This "struct" should be at the indent level of "typename" https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:106: RangeConstraint MakeRangeConstraint(bool is_in_upper_bound, MakeRangeConstraint() -> GetRangeConstraint() Make implies to me a delete... https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:107: bool is_in_lower_bound) { Change the enum definition to something that looks like a bit pattern rather than an ordinal. So enum RangeConstraint { RANGE_VALID = 0x0, RANGE_UNDERFLOW = 0x1, RANGE_OVERFLOW = 0x2, RANGE_INVALID = RANGE_UNDERFLOW | RANGE_OVERFLOW, } and/or add a DCHECK(RANGE_INVALID == RANGE_UNDERFLOW | RANGE_OVERFLOW) https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:123: struct DstRangeRelationToSrcRangeImpl; backup the struct to the typedef indentation level. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:129: // Dst range is statically determine to contain Src: Nothing to check. nit: determine -> determined https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:151: return std::numeric_limits<Dst>::is_iec559 Should this be using lowest()? Don't block submitting this CL on this question. I'm just trying to understand. The current logic looks correct. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:182: value <= static_cast<Src>(std::numeric_limits<Dst>::max()), Checking my understanding...the reason this cast is safe is because NUMERIC_RANGE_NOT_CONTAINED implies that Dst has a smaller range than Src, correct? https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:196: return (MaxExponent<Dst>::value >= MaxExponent<Src>::value) Okay, I don't understand, but I'll trust you did this right.
So I think my brain is fried from the templates, but AFAICT, the templates are OK, and you've got actionable feedback on how to collapse and eliminate the macros. Are you planning to land-and-optimize or are you wanting to optimize first? https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_mat... base/numerics/safe_math.h:52: friend class CheckedNumeric; indentation seems wonky on this. One line? template <typename Src> friend class CheckedNumeric; https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_mat... base/numerics/safe_math.h:93: T ValueUnsafe() const { return state_.value(); } Should you move these "Don't use in external code" to the bottom of the public section, so that it's "clear" they're "almost private" ? https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_mat... base/numerics/safe_math.h:119: CheckedNumeric& operator+=(Src rhs); same comments re: indenting above.
He should land-and-optimize. :) On Mon, Feb 24, 2014 at 3:37 PM, <rsleevi@chromium.org> wrote: > So I think my brain is fried from the templates, but AFAICT, the templates > are > OK, and you've got actionable feedback on how to collapse and eliminate the > macros. Are you planning to land-and-optimize or are you wanting to > optimize > first? > > > https://codereview.chromium.org/141583008/diff/1750001/ > base/numerics/safe_math.h > File base/numerics/safe_math.h (right): > > https://codereview.chromium.org/141583008/diff/1750001/ > base/numerics/safe_math.h#newcode52 > base/numerics/safe_math.h:52: friend class CheckedNumeric; > indentation seems wonky on this. One line? > > template <typename Src> friend class CheckedNumeric; > > https://codereview.chromium.org/141583008/diff/1750001/ > base/numerics/safe_math.h#newcode93 > base/numerics/safe_math.h:93: T ValueUnsafe() const { return > state_.value(); } > Should you move these "Don't use in external code" to the bottom of the > public section, so that it's "clear" they're "almost private" ? > > https://codereview.chromium.org/141583008/diff/1750001/ > base/numerics/safe_math.h#newcode119 > base/numerics/safe_math.h:119: CheckedNumeric& operator+=(Src rhs); > same comments re: indenting above. > > https://codereview.chromium.org/141583008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry for the delay. I think I've actioned all the feedback and adequately marked the TODOs. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... File base/numerics/safe_conversions_impl.h (right): https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:27: enum IntegerSignRepresentation { On 2014/02/21 22:27:41, awong wrote: > s/IntegerSignRepresentation/IntegerRepresentation/g Done. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:55: struct StaticDstRangeRelationToSrcRange; On 2014/02/21 22:27:41, awong wrote: > This "struct" should be at the indent level of "typename" Done. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:106: RangeConstraint MakeRangeConstraint(bool is_in_upper_bound, On 2014/02/21 22:27:41, awong wrote: > MakeRangeConstraint() -> GetRangeConstraint() > > Make implies to me a delete... Done. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:107: bool is_in_lower_bound) { On 2014/02/21 22:27:41, awong wrote: > Change the enum definition to something that looks like a bit pattern rather > than an ordinal. > > So > > enum RangeConstraint { > RANGE_VALID = 0x0, > RANGE_UNDERFLOW = 0x1, > RANGE_OVERFLOW = 0x2, > RANGE_INVALID = RANGE_UNDERFLOW | RANGE_OVERFLOW, > } > > and/or add a DCHECK(RANGE_INVALID == RANGE_UNDERFLOW | RANGE_OVERFLOW) Done. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:123: struct DstRangeRelationToSrcRangeImpl; On 2014/02/21 22:27:41, awong wrote: > backup the struct to the typedef indentation level. Done. Sorry, missed this one after the auto-format. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:129: // Dst range is statically determine to contain Src: Nothing to check. On 2014/02/21 22:27:41, awong wrote: > nit: determine -> determined Done. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:151: return std::numeric_limits<Dst>::is_iec559 On 2014/02/21 22:27:41, awong wrote: > Should this be using lowest()? > > Don't block submitting this CL on this question. I'm just trying to understand. > The current logic looks correct. It should, but lowest() is C++11, so I explicitly avoided it. That's also why I used numeric_limits instead of type traits in various places. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:182: value <= static_cast<Src>(std::numeric_limits<Dst>::max()), On 2014/02/21 22:27:41, awong wrote: > Checking my understanding...the reason this cast is safe is because > NUMERIC_RANGE_NOT_CONTAINED implies that Dst has a smaller range than Src, > correct? Yep. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_mat... base/numerics/safe_math.h:52: friend class CheckedNumeric; On 2014/02/24 23:37:44, Ryan Sleevi wrote: > indentation seems wonky on this. One line? > > template <typename Src> friend class CheckedNumeric; Done (but for the record, "git cl format" hates templates). https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_mat... base/numerics/safe_math.h:93: T ValueUnsafe() const { return state_.value(); } On 2014/02/24 23:37:44, Ryan Sleevi wrote: > Should you move these "Don't use in external code" to the bottom of the public > section, so that it's "clear" they're "almost private" ? Done. https://codereview.chromium.org/141583008/diff/1750001/base/numerics/safe_mat... base/numerics/safe_math.h:119: CheckedNumeric& operator+=(Src rhs); On 2014/02/24 23:37:44, Ryan Sleevi wrote: > same comments re: indenting above. Done.
3 more nits. https://codereview.chromium.org/141583008/diff/1860001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/141583008/diff/1860001/base/numerics/safe_mat... base/numerics/safe_math.h:46: private: Private goes at the end. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order https://codereview.chromium.org/141583008/diff/1860001/base/numerics/safe_mat... base/numerics/safe_math.h:51: template <typename Src> friend class CheckedNumeric; Shouldn't friend declarations go in the private section? https://codereview.chromium.org/141583008/diff/1860001/base/numerics/safe_mat... base/numerics/safe_math.h:58: : state_(rhs.state_.value(), rhs.state_.validity()) {} Can't we just do rhs.ValueUnsafe() and rhs.validity() thereby removing the need for friends?
https://codereview.chromium.org/141583008/diff/1860001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/141583008/diff/1860001/base/numerics/safe_mat... base/numerics/safe_math.h:46: private: On 2014/02/25 22:41:02, awong wrote: > Private goes at the end. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order Done. https://codereview.chromium.org/141583008/diff/1860001/base/numerics/safe_mat... base/numerics/safe_math.h:51: template <typename Src> friend class CheckedNumeric; On 2014/02/25 22:41:02, awong wrote: > Shouldn't friend declarations go in the private section? Done. https://codereview.chromium.org/141583008/diff/1860001/base/numerics/safe_mat... base/numerics/safe_math.h:58: : state_(rhs.state_.value(), rhs.state_.validity()) {} On 2014/02/25 22:41:02, awong wrote: > Can't we just do rhs.ValueUnsafe() and rhs.validity() thereby removing the need > for friends? Done.
LGTM. Ship it!
The CQ bit was checked by jschuh@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jschuh@chromium.org/141583008/1900001
The CQ bit was unchecked by jschuh@chromium.org
The CQ bit was checked by jschuh@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jschuh@chromium.org/141583008/1920001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_dbg_simulator for step(s) base_unittests, compile, components_unittests, content_unittests, crypto_unittests, net_unittests, sql_unittests, ui_unittests, url_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
The CQ bit was checked by jschuh@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jschuh@chromium.org/141583008/1920001
The CQ bit was checked by jschuh@chromium.org
The CQ bit was unchecked by jschuh@chromium.org
The CQ bit was checked by jschuh@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jschuh@chromium.org/141583008/1950001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_rel_device for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
The CQ bit was checked by jschuh@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jschuh@chromium.org/141583008/1970001
Message was sent while issue was closed.
Change committed as 253789
Message was sent while issue was closed.
eustas@chromium.org changed reviewers: + eustas@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/141583008/diff/1970001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/141583008/diff/1970001/base/numerics/safe_mat... base/numerics/safe_math.h:39: // CheckedNumeric<size_t> checked_size; "checked_size" redeclaration looks a little bit confusing... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
