Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2554803002:
Cleanup some undefined floating point behavior in base/numerics (Closed)
DescriptionCleanup some undefined floating point behavior in base/numerics
BUG=670885
Committed: https://crrev.com/7b594481a5452749f3abb50a5d82fd3fcf1c205c
Cr-Commit-Position: refs/heads/master@{#436746}
Patch Set 1 #
Total comments: 9
Patch Set 2 : fixed #
Total comments: 4
Patch Set 3 : nits #Patch Set 4 : compile fix #
Messages
Total messages: 28 (15 generated)
Patchset #1 (id:1) 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.
jschuh@chromium.org changed reviewers: + thakis@chromium.org
ptal
Thanks! https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_conv... File base/numerics/safe_conversions.h (right): https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_conv... base/numerics/safe_conversions.h:139: ? -std::numeric_limits<Dst>::infinity() I'm assuming you're static_assert()ing somewhere that our floating types satisfy std::numeric_limits::is_iec559. (in practice, they do, everywhere) https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_math... File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_math... base/numerics/safe_math_impl.h:643: return (std::is_integral<Src>::value || Nice! https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_math... base/numerics/safe_math_impl.h:645: NUMERIC_RANGE_CONTAINED || Isn't the is_integral check on Src stronger than this already? In this specialization, we know that Dst is integral. If src is not integral (i.e. float), this will never be true, right? And if Src is integral, the is_integral checks for that already. So maybe this part can just be omitted? https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_math... base/numerics/safe_math_impl.h:693: : value_((is_valid && IsValueInRangeForNumericType<T>(value)) Did you mean to replace these with saturating_cast? https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_math... base/numerics/safe_math_impl.h:699: : value_(static_cast<T>(value)) {} ditto https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_math... base/numerics/safe_math_impl.h:704: : value_(static_cast<T>(rhs.value())) {} ditto
Somehow I posted my WIP, but not the complete patch. I'll figure out what's going on a post the rest of the CL once I get back to my computer https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_conv... File base/numerics/safe_conversions.h (right): https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_conv... base/numerics/safe_conversions.h:139: ? -std::numeric_limits<Dst>::infinity() On 2016/12/06 14:03:37, Nico wrote: > I'm assuming you're static_assert()ing somewhere that our floating types satisfy > std::numeric_limits::is_iec559. (in practice, they do, everywhere) We have a ton of tests that would fail if they didn't. https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_math... File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_math... base/numerics/safe_math_impl.h:645: NUMERIC_RANGE_CONTAINED || On 2016/12/06 14:03:38, Nico wrote: > Isn't the is_integral check on Src stronger than this already? In this > specialization, we know that Dst is integral. If src is not integral (i.e. > float), this will never be true, right? And if Src is integral, the is_integral > checks for that already. So maybe this part can just be omitted? Weird. Somehow I posted this in an incomplete state, while I was splitting between floating point and int.
PTAL again. I have no idea how I messed up posting the original CL, but I just rewrote it on a plane. https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_math... File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/2554803002/diff/20001/base/numerics/safe_math... base/numerics/safe_math_impl.h:693: : value_((is_valid && IsValueInRangeForNumericType<T>(value)) On 2016/12/06 14:03:37, Nico wrote: > Did you mean to replace these with saturating_cast? Not exactly, but you were totally correct that the patch was missing something.
lgtm, thanks! https://codereview.chromium.org/2554803002/diff/40001/base/numerics/safe_conv... File base/numerics/safe_conversions.h (right): https://codereview.chromium.org/2554803002/diff/40001/base/numerics/safe_conv... base/numerics/safe_conversions.h:134: constexpr Dst saturated_cast_impl(const Src value, So this is unrelated to the change in the other file and a no-op? Or is it needed so that IsValueInRangeForNumericType does the right thing for floating args? https://codereview.chromium.org/2554803002/diff/40001/base/numerics/safe_math... File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/2554803002/diff/40001/base/numerics/safe_math... base/numerics/safe_math_impl.h:690: : static_cast<T>(std::numeric_limits<T>::quiet_NaN()); Do you need the cast here? numeric_limits<T>::quiet_NaN() already has type T, no?
https://codereview.chromium.org/2554803002/diff/40001/base/numerics/safe_conv... File base/numerics/safe_conversions.h (right): https://codereview.chromium.org/2554803002/diff/40001/base/numerics/safe_conv... base/numerics/safe_conversions.h:134: constexpr Dst saturated_cast_impl(const Src value, On 2016/12/06 17:15:50, Nico wrote: > So this is unrelated to the change in the other file and a no-op? Or is it > needed so that IsValueInRangeForNumericType does the right thing for floating > args? Unrelated. I'm naively hoping to fix all the float cases in one CL. https://codereview.chromium.org/2554803002/diff/40001/base/numerics/safe_math... File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/2554803002/diff/40001/base/numerics/safe_math... base/numerics/safe_math_impl.h:690: : static_cast<T>(std::numeric_limits<T>::quiet_NaN()); On 2016/12/06 17:15:50, Nico wrote: > Do you need the cast here? numeric_limits<T>::quiet_NaN() already has type T, > no? I do not, and that's what I get for hurriedly rewriting from memory.
The CQ bit was checked by jschuh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2554803002/#ps60001 (title: "nits")
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_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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 thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2554803002/#ps80001 (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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thakis@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": 80001, "attempt_start_ts": 1481055032871350,
"parent_rev": "a9a65b5d8a5fa17febea5362c1c544329fbd3163", "commit_rev":
"4f2f0c4ed67d4714d5e8ccae2420160f3ee28816"}
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Cleanup some undefined floating point behavior in base/numerics BUG=670885 ========== to ========== Cleanup some undefined floating point behavior in base/numerics BUG=670885 Committed: https://crrev.com/7b594481a5452749f3abb50a5d82fd3fcf1c205c Cr-Commit-Position: refs/heads/master@{#436746} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7b594481a5452749f3abb50a5d82fd3fcf1c205c Cr-Commit-Position: refs/heads/master@{#436746} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
