Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2522073004:
Add variadic helper functions for CheckedNumeric math operations (Closed)
DescriptionAdd variadic helper functions for CheckedNumeric math operations
This introduces variadic template functions that accept two or more
arithmetic or CheckedNumeric arguments and return a CheckedNumeric.
The functions are:
* base::CheckAdd() - addition
* base::CheckSub() - subtraction
* base::CheckMul() - multiplication
* base::CheckDiv() - division
* base::CheckMod() - modulous
* base::CheckLsh() - left integer shift
* base::CheckRsh() - right integer shift
BUG=667097
NOTRY=true
NOPRESUBMIT=true
Committed: https://crrev.com/4fcd6fa432b464f64de96c9bb03593b131a07029
Cr-Commit-Position: refs/heads/master@{#434311}
Patch Set 1 #Patch Set 2 : docs and nits #Patch Set 3 : clang/gcc compile fixes #Patch Set 4 : clang/gcc compile fix #Patch Set 5 : test fixes #
Total comments: 10
Patch Set 6 : nits #
Total comments: 1
Patch Set 7 : compile fix #
Messages
Total messages: 50 (25 generated)
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...
jschuh@chromium.org changed reviewers: + tsepez@chromium.org
tsepez@ - One last big CL full of ugly template goop. There's a little bit of weirdness necessary to avoid triggering bugs or unsupported language features in our various compilers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/11/23 14:59:02, jschuh (very slow) wrote: > tsepez@ - One last big CL full of ugly template goop. There's a little bit of > weirdness necessary to avoid triggering bugs or unsupported language features in > our various compilers. Looks good. Did somebody do a #define Op somewhere?
https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_math... base/numerics/safe_math.h:258: // The typedef was required here because MSVC fails to compile with "using". Let's get Bruce to weigh in on this one - at least push the issue upwards. https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_nume... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_nume... base/numerics/safe_numerics_unittest.cc:823: auto a = base::CheckedAdd(1, 2UL, 3LL, 4).ValueOrDie(); Can we throw a checked numeric argument into each of these cases?
https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_math... base/numerics/safe_math.h:38: // * CheckedAdd() - addition Can I explicitly specialize these, for instance if I need an int32_t and I'm adding a bunch of random crap, eg. int32_t x = CheckedAdd<int32_t>(....) with int64 types as args?
https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_math... base/numerics/safe_math.h:38: // * CheckedAdd() - addition On 2016/11/23 16:54:58, Tom Sepez wrote: > Can I explicitly specialize these, for instance if I need an int32_t and I'm > adding a bunch of random crap, eg. > > int32_t x = CheckedAdd<int32_t>(....) with int64 types as args? That's a good idea. I can add it. https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_math... base/numerics/safe_math.h:258: // The typedef was required here because MSVC fails to compile with "using". On 2016/11/23 16:52:06, Tom Sepez wrote: > Let's get Bruce to weigh in on this one - at least push the issue upwards. I've learned that any complex type resolution with using is kinda broken in MSVS. I've hit it in a few different places this week, and I was planning on following up with Bruce about it when I got back. https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_nume... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_nume... base/numerics/safe_numerics_unittest.cc:823: auto a = base::CheckedAdd(1, 2UL, 3LL, 4).ValueOrDie(); On 2016/11/23 16:52:06, Tom Sepez wrote: > Can we throw a checked numeric argument into each of these cases? Sounds good. I was going to add a simple wrapper template function for cases like this, so might as well do it here.
https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_math... base/numerics/safe_math.h:38: // * CheckedAdd() - addition On 2016/11/23 19:18:00, jschuh (very slow) wrote: > On 2016/11/23 16:54:58, Tom Sepez wrote: > > Can I explicitly specialize these, for instance if I need an int32_t and I'm > > adding a bunch of random crap, eg. > > > > int32_t x = CheckedAdd<int32_t>(....) with int64 types as args? > > That's a good idea. I can add it. Yeah, I kinda worry about something getting silently promoted to int64, and then passing that to an int32 function argument. https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_math... base/numerics/safe_math.h:38: // * CheckedAdd() - addition On 2016/11/23 19:18:00, jschuh (very slow) wrote: > On 2016/11/23 16:54:58, Tom Sepez wrote: > > Can I explicitly specialize these, for instance if I need an int32_t and I'm > > adding a bunch of random crap, eg. > > > > int32_t x = CheckedAdd<int32_t>(....) with int64 types as args? > > That's a good idea. I can add it. I guess I worry about something getting silently promoted to 64-bits, and then passing that to a smaller function argument. void frob(int32_t x) {} int32_t h, w; ... frob(CheckMul(h, w).ValueOrDefault(0)); Does that give a compilation error? Or is the result type int32_t?
Patchset #6 (id:100001) has been deleted
Note: CL Description needs update with short names.
https://codereview.chromium.org/2522073004/diff/120001/base/numerics/safe_num... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2522073004/diff/120001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:845: EXPECT_EQ(static_cast<decltype(h)>(1), h); can we add a test that CheckAdd<int16_t>(0x10000, 0x10000).ValueOrDefault(42) == 42.
When I'm back in the office we can talk about the problem of silent truncation when extracting the value from a CheckedNumeric. My original plan was to make some tweaks to StrictNumeric and use it as a proxy class in the return type. Unfortunately, that breaks auto variables. I might still do it that way anyway, and have been working on a CL, but I'm still trying to think through it. https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_math... base/numerics/safe_math.h:38: // * CheckedAdd() - addition On 2016/11/23 19:47:08, Tom Sepez wrote: > On 2016/11/23 19:18:00, jschuh (very slow) wrote: > > On 2016/11/23 16:54:58, Tom Sepez wrote: > > > Can I explicitly specialize these, for instance if I need an int32_t and I'm > > > adding a bunch of random crap, eg. > > > > > > int32_t x = CheckedAdd<int32_t>(....) with int64 types as args? > > > > That's a good idea. I can add it. > > Yeah, I kinda worry about something getting silently promoted to int64, and then > passing that to an int32 function argument. Okay, we might have a miscommunication here. I have another CL in progress with a proxy class to try to catch the scenario you're referring to. However, we've had that problem the whole time, so this adds no new risk on that front. Also, I want to honor the natural arithmetic promotions, so I don't want to require an explicit return type. And it turns out there's no nice way to make it optional with a function template, so I just added a Cast<T> method to CheckedNumeric, which serves the same purposes. https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_nume... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2522073004/diff/80001/base/numerics/safe_nume... base/numerics/safe_numerics_unittest.cc:823: auto a = base::CheckedAdd(1, 2UL, 3LL, 4).ValueOrDie(); On 2016/11/23 19:18:00, jschuh (very slow) wrote: > On 2016/11/23 16:52:06, Tom Sepez wrote: > > Can we throw a checked numeric argument into each of these cases? > > Sounds good. I was going to add a simple wrapper template function for cases > like this, so might as well do it here. Done.
Description was changed from ========== Add variadic helper functions for CheckedNumeric math operations This introduces variadic template functions that accept two or more arithmetic or CheckedNumeric arguments and return a CheckedNumeric. The functions are: * base::CheckedAdd() - addition * base::CheckedSub() - subtraction * base::CheckedMul() - multiplication * base::CheckedDiv() - division * base::CheckedMod() - modulous * base::CheckedLsh() - left integer shift * base::CheckedRsh() - right integer shift BUG=667097 ========== to ========== Add variadic helper functions for CheckedNumeric math operations This introduces variadic template functions that accept two or more arithmetic or CheckedNumeric arguments and return a CheckedNumeric. The functions are: * base::CheckAdd() - addition * base::CheckSub() - subtraction * base::CheckMul() - multiplication * base::CheckDiv() - division * base::CheckMod() - modulous * base::CheckLsh() - left integer shift * base::CheckRsh() - right integer shift BUG=667097 ==========
lgtm
The CQ bit was checked by jschuh@chromium.org
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by jschuh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2522073004/#ps140001 (title: "compile fix")
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by jschuh@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by jschuh@chromium.org
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 jschuh@chromium.org
Description was changed from ========== Add variadic helper functions for CheckedNumeric math operations This introduces variadic template functions that accept two or more arithmetic or CheckedNumeric arguments and return a CheckedNumeric. The functions are: * base::CheckAdd() - addition * base::CheckSub() - subtraction * base::CheckMul() - multiplication * base::CheckDiv() - division * base::CheckMod() - modulous * base::CheckLsh() - left integer shift * base::CheckRsh() - right integer shift BUG=667097 ========== to ========== Add variadic helper functions for CheckedNumeric math operations This introduces variadic template functions that accept two or more arithmetic or CheckedNumeric arguments and return a CheckedNumeric. The functions are: * base::CheckAdd() - addition * base::CheckSub() - subtraction * base::CheckMul() - multiplication * base::CheckDiv() - division * base::CheckMod() - modulous * base::CheckLsh() - left integer shift * base::CheckRsh() - right integer shift BUG=667097 NOTRY=true ==========
The CQ bit was checked by jschuh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add variadic helper functions for CheckedNumeric math operations This introduces variadic template functions that accept two or more arithmetic or CheckedNumeric arguments and return a CheckedNumeric. The functions are: * base::CheckAdd() - addition * base::CheckSub() - subtraction * base::CheckMul() - multiplication * base::CheckDiv() - division * base::CheckMod() - modulous * base::CheckLsh() - left integer shift * base::CheckRsh() - right integer shift BUG=667097 NOTRY=true ========== to ========== Add variadic helper functions for CheckedNumeric math operations This introduces variadic template functions that accept two or more arithmetic or CheckedNumeric arguments and return a CheckedNumeric. The functions are: * base::CheckAdd() - addition * base::CheckSub() - subtraction * base::CheckMul() - multiplication * base::CheckDiv() - division * base::CheckMod() - modulous * base::CheckLsh() - left integer shift * base::CheckRsh() - right integer shift BUG=667097 NOTRY=true NOPRESUBMIT=true ==========
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-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1479988667571960,
"parent_rev": "c37974c615f4e6fcff7a438e209b686d336923b0", "commit_rev":
"92eeea7822f42323c3ced8c23fbfe130adaeb1cb"}
Message was sent while issue was closed.
Description was changed from ========== Add variadic helper functions for CheckedNumeric math operations This introduces variadic template functions that accept two or more arithmetic or CheckedNumeric arguments and return a CheckedNumeric. The functions are: * base::CheckAdd() - addition * base::CheckSub() - subtraction * base::CheckMul() - multiplication * base::CheckDiv() - division * base::CheckMod() - modulous * base::CheckLsh() - left integer shift * base::CheckRsh() - right integer shift BUG=667097 NOTRY=true NOPRESUBMIT=true ========== to ========== Add variadic helper functions for CheckedNumeric math operations This introduces variadic template functions that accept two or more arithmetic or CheckedNumeric arguments and return a CheckedNumeric. The functions are: * base::CheckAdd() - addition * base::CheckSub() - subtraction * base::CheckMul() - multiplication * base::CheckDiv() - division * base::CheckMod() - modulous * base::CheckLsh() - left integer shift * base::CheckRsh() - right integer shift BUG=667097 NOTRY=true NOPRESUBMIT=true ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add variadic helper functions for CheckedNumeric math operations This introduces variadic template functions that accept two or more arithmetic or CheckedNumeric arguments and return a CheckedNumeric. The functions are: * base::CheckAdd() - addition * base::CheckSub() - subtraction * base::CheckMul() - multiplication * base::CheckDiv() - division * base::CheckMod() - modulous * base::CheckLsh() - left integer shift * base::CheckRsh() - right integer shift BUG=667097 NOTRY=true NOPRESUBMIT=true ========== to ========== Add variadic helper functions for CheckedNumeric math operations This introduces variadic template functions that accept two or more arithmetic or CheckedNumeric arguments and return a CheckedNumeric. The functions are: * base::CheckAdd() - addition * base::CheckSub() - subtraction * base::CheckMul() - multiplication * base::CheckDiv() - division * base::CheckMod() - modulous * base::CheckLsh() - left integer shift * base::CheckRsh() - right integer shift BUG=667097 NOTRY=true NOPRESUBMIT=true Committed: https://crrev.com/4fcd6fa432b464f64de96c9bb03593b131a07029 Cr-Commit-Position: refs/heads/master@{#434311} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4fcd6fa432b464f64de96c9bb03593b131a07029 Cr-Commit-Position: refs/heads/master@{#434311}
Message was sent while issue was closed.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
Message was sent while issue was closed.
Dumb question: what are the promotion rules with the new helpers? If I CheckAdd() arguments with a bunch of different widths, what width should I expect the result to be? In addition, what happens if I explicitly specialize the template to something that's smaller than some of the input arguments? I assume that works as well (and similarly for the case where I intentionally specialize to an overly wide width)?
Message was sent while issue was closed.
On 2016/12/01 07:40:22, dcheng wrote: > Dumb question: what are the promotion rules with the new helpers? If I > CheckAdd() arguments with a bunch of different widths, what width should I > expect the result to be? It's identical to how the CheckedNumeric arithmetic operator overloads work. That is, it's standard C arithmetic promotions with two big exceptions: 1. There's no default int promotion for types narrower than int. 2. Bitwise logical operations will always promote to unsigned of the wider type. > In addition, what happens if I explicitly specialize the template to something > that's smaller than some of the input arguments? I assume that works as well > (and similarly for the case where I intentionally specialize to an overly wide > width)? You'll get truncations or extensions, respectively, so I wouldn't advise it. I suppose this is a reasonable argument for a presubmit check. Also, FYI, if you want to safely force an arithmetic value to a wider type, you should use base::strict_cast<> because it will throw a compiler error on truncation.
Message was sent while issue was closed.
Can we document this in a comment? ;-) Daniel On Thu, Dec 1, 2016, 06:13 <jschuh@chromium.org> wrote: > On 2016/12/01 07:40:22, dcheng wrote: > > Dumb question: what are the promotion rules with the new helpers? If I > > CheckAdd() arguments with a bunch of different widths, what width should > I > > expect the result to be? > > It's identical to how the CheckedNumeric arithmetic operator overloads > work. > That is, it's standard C arithmetic promotions with two big exceptions: > 1. There's no default int promotion for types narrower than int. > 2. Bitwise logical operations will always promote to unsigned of the wider > type. > > > > In addition, what happens if I explicitly specialize the template to > something > > that's smaller than some of the input arguments? I assume that works as > well > > (and similarly for the case where I intentionally specialize to an > overly wide > > width)? > > You'll get truncations or extensions, respectively, so I wouldn't advise > it. I > suppose this is a reasonable argument for a presubmit check. > > Also, FYI, if you want to safely force an arithmetic value to a wider > type, you > should use base::strict_cast<> because it will throw a compiler error on > truncation. > > https://codereview.chromium.org/2522073004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
I will do this thing you ask of me... but I will make you review the CL. On 2016/12/01 16:08:09, chromium-reviews wrote: > Can we document this in a comment? ;-) > > Daniel > > On Thu, Dec 1, 2016, 06:13 <mailto:jschuh@chromium.org> wrote: > > > On 2016/12/01 07:40:22, dcheng wrote: > > > Dumb question: what are the promotion rules with the new helpers? If I > > > CheckAdd() arguments with a bunch of different widths, what width should > > I > > > expect the result to be? > > > > It's identical to how the CheckedNumeric arithmetic operator overloads > > work. > > That is, it's standard C arithmetic promotions with two big exceptions: > > 1. There's no default int promotion for types narrower than int. > > 2. Bitwise logical operations will always promote to unsigned of the wider > > type. > > > > > > > In addition, what happens if I explicitly specialize the template to > > something > > > that's smaller than some of the input arguments? I assume that works as > > well > > > (and similarly for the case where I intentionally specialize to an > > overly wide > > > width)? > > > > You'll get truncations or extensions, respectively, so I wouldn't advise > > it. I > > suppose this is a reasonable argument for a presubmit check. > > > > Also, FYI, if you want to safely force an arithmetic value to a wider > > type, you > > should use base::strict_cast<> because it will throw a compiler error on > > truncation. > > > > https://codereview.chromium.org/2522073004/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
