Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2945433003:
Add ClampedNumeric templates (Closed)
DescriptionAdd ClampedNumeric templates
Add template classes for clamped (not sticky) saturating math.
BUG=672489
Review-Url: https://codereview.chromium.org/2945433003
Cr-Commit-Position: refs/heads/master@{#484005}
Committed: https://chromium.googlesource.com/chromium/src/+/b180f39aa6a2edc68a5c0e2308aabe639ec029f8
Patch Set 1 #Patch Set 2 : wip #Patch Set 3 : mostly done and added tests #Patch Set 4 : docs, nits, remove asm #Patch Set 5 : more docs and modulus #
Total comments: 27
Patch Set 6 : post-review fixes #
Total comments: 35
Patch Set 7 : feedback round 2 #
Total comments: 6
Patch Set 8 : final #
Messages
Total messages: 31 (21 generated)
Patchset #4 (id:60001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by jschuh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Patchset #4 (id:160001) has been deleted
The CQ bit was checked by jschuh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add ClampedNumeric templates Add template classes for clamped (not sticky) saturating math. BUG=672489 ========== to ========== Add ClampedNumeric templates Add template classes for clamped (not sticky) saturating math. BUG=672489 ==========
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
The big changes are in these files: * base/numerics/clamped_math.h * base/numerics/clamped_math_impl.h And all the tests for new functionality are here: * base/numerics/safe_numerics_unittest.cc The other big change is expanding the documentation and moving it to markdown: * base/numerics/README.md I've added comments inline to cover some corner case bugs that I discovered in the process. The rest of the stuff should be self explanatory. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/checked_... File base/numerics/checked_math_impl.h (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/checked_... base/numerics/checked_math_impl.h:189: return false; The only functional change here is supporting the case where either operand is zero, which is hit by some of the ClampedNumeric uses. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/checked_... base/numerics/checked_math_impl.h:294: return !x ? (*result = V(0)), true : false; Stumbled on a spec error in how I was handling zero, so I chose to clean it up in this CL in order to match the ClampedNumeric behavior. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/checked_... base/numerics/checked_math_impl.h:311: if (!x || static_cast<ShiftType>(shift) < IntegerBitsPlusSign<T>::value) { Stumbled on a spec error in how I was handling zero, so I had to clean it up because the ClampedNumeric implementation relies on this function. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_con... File base/numerics/safe_conversions.h (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_con... base/numerics/safe_conversions.h:232: #define BASE_NUMERIC_COMPARISON_OPERATORS(CLASS, NAME, OP) \ Generalized the implementation so ClampedNumeric can use it. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_mat... File base/numerics/safe_math_shared_impl.h (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_mat... base/numerics/safe_math_shared_impl.h:117: #define BASE_NUMERIC_ARITHMETIC_VARIADIC(CLASS, CL_ABBR, OP_NAME) \ Finished generalizing the implementation so ClampedNumeric can use it.
Oh, one more piece of context. After this CL I have to land some assembler implementations of the hotter clamped arithmetic operations, and then I'm going to rip out the base/numerics/saturated_arithmetic* functions and replace them with this.
I'm still reviewing clamped_math* in detail (sorry!) but some initial comments to get things started. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/README.md File base/numerics/README.md (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/README.m... base/numerics/README.md:32: of the same width. I think I'm being unimaginative, but to me, I'm not sure why these would be in the safe numerics library. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/checked_... File base/numerics/checked_math_impl.h (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/checked_... base/numerics/checked_math_impl.h:294: return !x ? (*result = V(0)), true : false; On 2017/06/26 20:26:50, jschuh wrote: > Stumbled on a spec error in how I was handling zero, so I chose to clean it up > in this CL in order to match the ClampedNumeric behavior. Acknowledged--though for future CLs, it would be nice to split fixes like this out into a separate CL (since it makes it easier to review any associated test changes). Dependent CLs are really easy in Gerrit ;-) That being said... here's what I found in the C++11 spec in [expr.shift]/1: The behavior is undefined if the right operand is negative, or greater than or equal to the length in bits of the promoted left operand. I don't see any caveat here for 0? https://codereview.chromium.org/2945433003/diff/200001/base/numerics/checked_... base/numerics/checked_math_impl.h:311: if (!x || static_cast<ShiftType>(shift) < IntegerBitsPlusSign<T>::value) { On 2017/06/26 20:26:50, jschuh wrote: > Stumbled on a spec error in how I was handling zero, so I had to clean it up > because the ClampedNumeric implementation relies on this function. Ditto. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/clamped_... File base/numerics/clamped_math.h (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/clamped_... base/numerics/clamped_math.h:52: // ClampedNumeric. If the current state is invalid or the destination cannot "current state is invalid" => how does that happen for a clamped numeric? https://codereview.chromium.org/2945433003/diff/200001/base/numerics/clamped_... base/numerics/clamped_math.h:152: template <template <typename, typename, typename> class M, TBH, I found these templates pretty hard to read due to all the deduced parameters that aren't explicitly named here. I'm kind of guessing here, since I'm still reading the CL, but from what I can gather, M is also a MathWrapper? https://codereview.chromium.org/2945433003/diff/200001/base/numerics/clamped_... base/numerics/clamped_math.h:222: return ClampMathOp<M>(ClampMathOp<M>(lhs, rhs), args...); Frightening =) https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_con... File base/numerics/safe_conversions.h (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_con... base/numerics/safe_conversions.h:85: // static constexpr T required() is required by the library. required() => max()? Also I don't quite understand this comment--required by what library? https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_con... File base/numerics/safe_conversions_impl.h (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:595: !(UnderlyingType<L>::is_checked || UnderlyingType<R>::is_checked); This confused me a bit initially--why would we have to filter these bits out if each type trait only sets one of the corresponding bits? I think this is because we have some implicit conversions allowed between safe numeric types--perhaps it's worth documenting them in the README.md as well https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_num... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:25: using base::ClampedNumeric; FWIW, I think we usually dump tests into the same namespace as they're testing for this reason. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:1551: auto f = ClampLsh(1, MakeClampedNum(2U)); How come this needs an explicit U while the checked num version doesn't?
Thanks for the review. Hopefully this covers everything. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/README.md File base/numerics/README.md (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/README.m... base/numerics/README.md:32: of the same width. On 2017/06/28 07:25:06, dcheng (OOO Jun 25 - Jul 1) wrote: > I think I'm being unimaginative, but to me, I'm not sure why these would be in > the safe numerics library. There's a number of places where we require a signed or unsigned type (e.g. the shift operators for the ClampedNumeric). Also, I use them internally, and they're handy enough that I figured I'd export them. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/checked_... File base/numerics/checked_math_impl.h (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/checked_... base/numerics/checked_math_impl.h:294: return !x ? (*result = V(0)), true : false; On 2017/06/28 07:25:06, dcheng (OOO Jun 25 - Jul 1) wrote: > On 2017/06/26 20:26:50, jschuh wrote: > > Stumbled on a spec error in how I was handling zero, so I chose to clean it up > > in this CL in order to match the ClampedNumeric behavior. > > Acknowledged--though for future CLs, it would be nice to split fixes like this > out into a separate CL (since it makes it easier to review any associated test > changes). Dependent CLs are really easy in Gerrit ;-) > > That being said... here's what I found in the C++11 spec in [expr.shift]/1: > The behavior is undefined if the right operand is negative, or greater than or > equal to the length in bits of the promoted left operand. > > I don't see any caveat here for 0? Weird. I'm looking at it now and you're right. Maybe I was looking at a working draft that was later changed. I'll fix it and the associated tests. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/checked_... base/numerics/checked_math_impl.h:311: if (!x || static_cast<ShiftType>(shift) < IntegerBitsPlusSign<T>::value) { On 2017/06/28 07:25:06, dcheng (OOO Jun 25 - Jul 1) wrote: > On 2017/06/26 20:26:50, jschuh wrote: > > Stumbled on a spec error in how I was handling zero, so I had to clean it up > > because the ClampedNumeric implementation relies on this function. > > Ditto. Yup. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/clamped_... File base/numerics/clamped_math.h (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/clamped_... base/numerics/clamped_math.h:52: // ClampedNumeric. If the current state is invalid or the destination cannot On 2017/06/28 07:25:06, dcheng (OOO Jun 25 - Jul 1) wrote: > "current state is invalid" => how does that happen for a clamped numeric? Oops. Copy pasta. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/clamped_... base/numerics/clamped_math.h:152: template <template <typename, typename, typename> class M, On 2017/06/28 07:25:06, dcheng (OOO Jun 25 - Jul 1) wrote: > TBH, I found these templates pretty hard to read due to all the deduced > parameters that aren't explicitly named here. I'm kind of guessing here, since > I'm still reading the CL, but from what I can gather, M is also a MathWrapper? It's the Clamped*Op struct. But yeah, I just used "M" for "math". Maybe "Op" would have been better? https://codereview.chromium.org/2945433003/diff/200001/base/numerics/clamped_... base/numerics/clamped_math.h:222: return ClampMathOp<M>(ClampMathOp<M>(lhs, rhs), args...); On 2017/06/28 07:25:06, dcheng (OOO Jun 25 - Jul 1) wrote: > Frightening =) But I figured out how to make variadic templates! It's my greatest developer accomplishment!!! https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_con... File base/numerics/safe_conversions.h (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_con... base/numerics/safe_conversions.h:85: // static constexpr T required() is required by the library. On 2017/06/28 07:25:06, dcheng (OOO Jun 25 - Jul 1) wrote: > required() => max()? > > Also I don't quite understand this comment--required by what library? D'oh! Typo. But the gist is that you can provide a custom version of this class in a few places to define custom limits (e.g. for saturated_cast). These comments call out the other members you're required to implement if you do. I can add a comment better explaining this. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_con... File base/numerics/safe_conversions_impl.h (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:595: !(UnderlyingType<L>::is_checked || UnderlyingType<R>::is_checked); On 2017/06/28 07:25:06, dcheng (OOO Jun 25 - Jul 1) wrote: > This confused me a bit initially--why would we have to filter these bits out if > each type trait only sets one of the corresponding bits? > > I think this is because we have some implicit conversions allowed between safe > numeric types--perhaps it's worth documenting them in the README.md as well I'll add it to the README. The gist is, if it can be cast directly to the underlying type then it can be up-converted. And priority of conversion is: StrictNumeric -> ClampedNumeric -> CheckedNumeric. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_num... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:25: using base::ClampedNumeric; On 2017/06/28 07:25:06, dcheng (OOO Jun 25 - Jul 1) wrote: > FWIW, I think we usually dump tests into the same namespace as they're testing > for this reason. Now you tell me. https://codereview.chromium.org/2945433003/diff/200001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:1551: auto f = ClampLsh(1, MakeClampedNum(2U)); On 2017/06/28 07:25:06, dcheng (OOO Jun 25 - Jul 1) wrote: > How come this needs an explicit U while the checked num version doesn't? Conscious API decision. Negative shift is always wrong, but for maximum flexibility the checked types can accept it and just treat it as an error. Whereas I couldn't figure out what the saturated types should do (since they can't throw an error) so I just require unsigned or trigger a compile error.
https://codereview.chromium.org/2945433003/diff/200001/base/numerics/README.md File base/numerics/README.md (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/README.m... base/numerics/README.md:32: of the same width. On 2017/06/28 12:32:37, jschuh wrote: > On 2017/06/28 07:25:06, dcheng (OOO Jun 25 - Jul 1) wrote: > > I think I'm being unimaginative, but to me, I'm not sure why these would be in > > the safe numerics library. > > There's a number of places where we require a signed or unsigned type (e.g. the > shift operators for the ClampedNumeric). Also, I use them internally, and > they're handy enough that I figured I'd export them. Hmm... I guess they just don't feel very "safe mathy" to me: it's not clear to me how this is safer than the standard way of writing it though (though it is a convenient shorthand) https://codereview.chromium.org/2945433003/diff/200001/base/numerics/clamped_... File base/numerics/clamped_math.h (right): https://codereview.chromium.org/2945433003/diff/200001/base/numerics/clamped_... base/numerics/clamped_math.h:152: template <template <typename, typename, typename> class M, On 2017/06/28 12:32:37, jschuh wrote: > On 2017/06/28 07:25:06, dcheng (OOO Jun 25 - Jul 1) wrote: > > TBH, I found these templates pretty hard to read due to all the deduced > > parameters that aren't explicitly named here. I'm kind of guessing here, since > > I'm still reading the CL, but from what I can gather, M is also a MathWrapper? > > It's the Clamped*Op struct. But yeah, I just used "M" for "math". Maybe "Op" > would have been better? Yeah I think that might be better. I would be OK doing this in a followup (to make things more consistent for CheckedNumerics as well) https://codereview.chromium.org/2945433003/diff/220001/base/numerics/checked_... File base/numerics/checked_math_impl.h (right): https://codereview.chromium.org/2945433003/diff/220001/base/numerics/checked_... base/numerics/checked_math_impl.h:290: as_unsigned(shift) < IntegerBitsPlusSign<T>::value) { Sorry, one more question: why do we silently convert shift to unsigned? C++ calls this out as a case of undefined behavior. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/checked_... base/numerics/checked_math_impl.h:318: if (as_unsigned(shift) < IntegerBitsPlusSign<T>::value) { Similar question here. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... File base/numerics/clamped_math.h (right): https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... base/numerics/clamped_math.h:84: NegateWrapper(value_) != std::numeric_limits<T>::lowest()) I can't help but feel that NegateWrapper / InvertWrapper / AbsWrapper could use some comments describing how they work / how callers are expected to check for underflow/overflow conditions. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... File base/numerics/clamped_math_impl.h (right): https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... base/numerics/clamped_math_impl.h:50: (IsValueConstantAndNegative(x) || IsValueNegative(y))); This is glorious and terrible. P.S. I think these could use some comments: while the results are "obvious" if you think about it enough, some comments wouldn't hurt. Throwing in "is constant and negative" adds an additional twist to reason through, even though the result is the same. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... base/numerics/clamped_math_impl.h:69: (IsValueConstantAndNegative(x) || !IsValueNegative(y))); Ditto. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... base/numerics/clamped_math_impl.h:141: static_assert(!std::is_signed<U>::value, "Shift value must be unsigned."); Oh... I guess this static assert is supposed to prevent negative inputs to shift (sorry for reading the files out of order) https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_con... File base/numerics/safe_conversions.h (right): https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_con... base/numerics/safe_conversions.h:87: // static constexpr T max() is inherited from std::numeric_limits. One alternative to the comment is to explicit write: using numeric_limits::max; etc https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:403: Is it worth adding some test cases for shifting a negative number? https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:527: << as_unsigned(IntegerBitsPlusSign<Dst>::value)); Part of me wonders why the trait for IntegerBitsPlusSign isn't unsigned to begin with. Also, this appears to be a duplicate of line 529. Was this meant to be another test originally? https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:544: 0, ClampedNumeric<Dst>(1) >> (IntegerBitsPlusSign<Dst>::value - 1U)); I guess this is to test that it works as expected no matter if the bits to shift by is signed or unsigned? https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:550: MakeCheckedNum(DstLimits::max()) & -1); I think this should be MakeClampedNum. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:604: ClampedNumeric<Dst>(DstLimits::max()) + 1); Is this checking that the result is +infinity/-infinity? https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:607: ClampedNumeric<Dst>(DstLimits::lowest()) + DstLimits::lowest()); I'm guessing it is, but I'm honestly not sure, since I would think that the expected value should be the same for both. Some comments would probably help =) https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:736: ClampedNumeric<Dst>(DstLimits::max()) - 1); I wonder why the CheckedNumeric version doesn't check the expected value too? https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:772: ClampedNumeric<uintmax_t>(-2)); Sanity check: ClampedNumeric<uintmax_t>(-2) == 0U, right? https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:791: Should we be checking division by zero here as well (for checked + clamped)? https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:941: TEST_EXPECTED_VALUE(Dst(SrcLimits::max()), clamped_dst); Ditto: I wonder why the checked version only checks success, but not the expected value, while the clamped version checks the expected value too (this applies to the tests below as well) https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:993: TEST_EXPECTED_VALUE(DstLimits::Overflow(), clamped_dst + SrcLimits::max()); One general thought about these tests is that they're somewhat opaque and hard to understand: I have to know that this only applies to src => dst conversions, when dst is smaller--but that's not really apparently unless I notice the static_assert on line 982. No real ideas on how to improve this though...
Patchset #7 (id:240001) has been deleted
Back at ya. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/checked_... File base/numerics/checked_math_impl.h (right): https://codereview.chromium.org/2945433003/diff/220001/base/numerics/checked_... base/numerics/checked_math_impl.h:290: as_unsigned(shift) < IntegerBitsPlusSign<T>::value) { On 2017/06/30 07:39:30, dcheng (OOO Jun 25 - Jul 1) wrote: > Sorry, one more question: why do we silently convert shift to unsigned? C++ > calls this out as a case of undefined behavior. Conversion from signed to unsigned is well-defined, and doing so means we don't need a separate check for values less than zero. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/checked_... base/numerics/checked_math_impl.h:318: if (as_unsigned(shift) < IntegerBitsPlusSign<T>::value) { On 2017/06/30 07:39:30, dcheng (OOO Jun 25 - Jul 1) wrote: > Similar question here. Same answer. It's well defined and saves a bounds check. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... File base/numerics/clamped_math.h (right): https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... base/numerics/clamped_math.h:84: NegateWrapper(value_) != std::numeric_limits<T>::lowest()) On 2017/06/30 07:39:30, dcheng (OOO Jun 25 - Jul 1) wrote: > I can't help but feel that NegateWrapper / InvertWrapper / AbsWrapper could use > some comments describing how they work / how callers are expected to check for > underflow/overflow conditions. There already is a comment at the function definition that I think is sufficient, and explicitly states that the caller must check for overflow. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... File base/numerics/clamped_math_impl.h (right): https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... base/numerics/clamped_math_impl.h:50: (IsValueConstantAndNegative(x) || IsValueNegative(y))); On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > This is glorious and terrible. > > P.S. I think these could use some comments: while the results are "obvious" if > you think about it enough, some comments wouldn't hurt. Throwing in "is constant > and negative" adds an additional twist to reason through, even though the result > is the same. Done. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... base/numerics/clamped_math_impl.h:69: (IsValueConstantAndNegative(x) || !IsValueNegative(y))); On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > Ditto. Done. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_con... File base/numerics/safe_conversions.h (right): https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_con... base/numerics/safe_conversions.h:87: // static constexpr T max() is inherited from std::numeric_limits. On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > One alternative to the comment is to explicit write: > > using numeric_limits::max; > > etc Done. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:403: On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > Is it worth adding some test cases for shifting a negative number? Done. nice catch. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:527: << as_unsigned(IntegerBitsPlusSign<Dst>::value)); On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > Part of me wonders why the trait for IntegerBitsPlusSign isn't unsigned to begin > with. > > Also, this appears to be a duplicate of line 529. Was this meant to be another > test originally? Fixed. Copy pasta. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:544: 0, ClampedNumeric<Dst>(1) >> (IntegerBitsPlusSign<Dst>::value - 1U)); On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > I guess this is to test that it works as expected no matter if the bits to shift > by is signed or unsigned? Yeah, just checking that the normal max legal shift works. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:550: MakeCheckedNum(DstLimits::max()) & -1); On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > I think this should be MakeClampedNum. Done. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:604: ClampedNumeric<Dst>(DstLimits::max()) + 1); On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > Is this checking that the result is +infinity/-infinity? Yeah, just checking that the wrapped floats behave as normal floats. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:736: ClampedNumeric<Dst>(DstLimits::max()) - 1); On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > I wonder why the CheckedNumeric version doesn't check the expected value too? The computation is just normal C/C++ arithmetic in pretty much all cases. The magic is in determining if the result of that computation is valid, and if there was a notion of "is valid" for clamped math I would have likely used that. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:772: ClampedNumeric<uintmax_t>(-2)); On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > Sanity check: ClampedNumeric<uintmax_t>(-2) == 0U, right? Yes. This is mainly checking propagation of the saturated result. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:791: On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > Should we be checking division by zero here as well (for checked + clamped)? Done. And nice catch, because it caught a corner case. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:941: TEST_EXPECTED_VALUE(Dst(SrcLimits::max()), clamped_dst); On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > Ditto: I wonder why the checked version only checks success, but not the > expected value, while the clamped version checks the expected value too (this > applies to the tests below as well) Ditto response from above. https://codereview.chromium.org/2945433003/diff/220001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:993: TEST_EXPECTED_VALUE(DstLimits::Overflow(), clamped_dst + SrcLimits::max()); On 2017/06/30 07:39:31, dcheng (OOO Jun 25 - Jul 1) wrote: > One general thought about these tests is that they're somewhat opaque and hard > to understand: I have to know that this only applies to src => dst conversions, > when dst is smaller--but that's not really apparently unless I notice the > static_assert on line 982. No real ideas on how to improve this though... Yeah. It's confusing stuff and I don't have a good idea on how to make the tests clearer.
🎆 LGTM 🎆 https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... File base/numerics/clamped_math.h (right): https://codereview.chromium.org/2945433003/diff/220001/base/numerics/clamped_... base/numerics/clamped_math.h:84: NegateWrapper(value_) != std::numeric_limits<T>::lowest()) On 2017/07/01 06:31:59, jschuh wrote: > On 2017/06/30 07:39:30, dcheng (OOO Jun 25 - Jul 1) wrote: > > I can't help but feel that NegateWrapper / InvertWrapper / AbsWrapper could > use > > some comments describing how they work / how callers are expected to check for > > underflow/overflow conditions. > > There already is a comment at the function definition that I think is > sufficient, and explicitly states that the caller must check for overflow. Right, the comments I was thinking of were more along the lines of how to check for overflow (and any interesting asides about twos complement math), etc. The answer is fairly self-evident if you think about it... but as it involves some clever code, sprinkling in some comments won't hurt. https://codereview.chromium.org/2945433003/diff/260001/base/numerics/README.md File base/numerics/README.md (right): https://codereview.chromium.org/2945433003/diff/260001/base/numerics/README.m... base/numerics/README.md:205: limit (max/min). 0 shifted any amount results in 0. Nit: maybe call out that a single shift > bit width is defined as well, since they aren't in normal C++ https://codereview.chromium.org/2945433003/diff/260001/base/numerics/safe_num... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2945433003/diff/260001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:404: -1, ClampedNumeric<Dst>(-1) >> (IntegerBitsPlusSign<Dst>::value - 1U)); Nit: add a test case that lowest() saturates to -1? https://codereview.chromium.org/2945433003/diff/260001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:793: TEST_EXPECTED_VALUE(0, ClampedNumeric<Dst>(0) / 0); Please update the documentation for clamped division to mention this case.
https://codereview.chromium.org/2945433003/diff/260001/base/numerics/README.md File base/numerics/README.md (right): https://codereview.chromium.org/2945433003/diff/260001/base/numerics/README.m... base/numerics/README.md:205: limit (max/min). 0 shifted any amount results in 0. On 2017/07/03 10:01:44, dcheng (OOO Jun 25 - Jul 1) wrote: > Nit: maybe call out that a single shift > bit width is defined as well, since > they aren't in normal C++ Done. https://codereview.chromium.org/2945433003/diff/260001/base/numerics/safe_num... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2945433003/diff/260001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:404: -1, ClampedNumeric<Dst>(-1) >> (IntegerBitsPlusSign<Dst>::value - 1U)); On 2017/07/03 10:01:44, dcheng (OOO Jun 25 - Jul 1) wrote: > Nit: add a test case that lowest() saturates to -1? Done. https://codereview.chromium.org/2945433003/diff/260001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:793: TEST_EXPECTED_VALUE(0, ClampedNumeric<Dst>(0) / 0); On 2017/07/03 10:01:44, dcheng (OOO Jun 25 - Jul 1) wrote: > Please update the documentation for clamped division to mention this case. Done.
The CQ bit was checked by jschuh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2945433003/#ps280001 (title: "final")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1499087034860540,
"parent_rev": "8348bba2aa459d465e7d3e9a26ff49cbc69f28ca", "commit_rev":
"b180f39aa6a2edc68a5c0e2308aabe639ec029f8"}
Message was sent while issue was closed.
Description was changed from ========== Add ClampedNumeric templates Add template classes for clamped (not sticky) saturating math. BUG=672489 ========== to ========== Add ClampedNumeric templates Add template classes for clamped (not sticky) saturating math. BUG=672489 Review-Url: https://codereview.chromium.org/2945433003 Cr-Commit-Position: refs/heads/master@{#484005} Committed: https://chromium.googlesource.com/chromium/src/+/b180f39aa6a2edc68a5c0e2308aa... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:280001) as https://chromium.googlesource.com/chromium/src/+/b180f39aa6a2edc68a5c0e2308aa... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
