Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2003693002:
Simplify validity detection in SafeNumerics tests (Closed)
|
Created:
4 years, 7 months ago by jschuh Modified:
4 years, 7 months ago Reviewers:
brucedawson CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSimplify validity detection in SafeNumerics tests
Intrinsics don't identify overflow direction, and we're not relying on
it anyway. So, it's easiest to remove it piece by piece.
BUG=613003
Committed: https://crrev.com/819c82686cc9d3ffb14db3f9926f92dcb50ab910
Cr-Commit-Position: refs/heads/master@{#395224}
Patch Set 1 #
Total comments: 2
Patch Set 2 : macros #
Total comments: 2
Patch Set 3 : oops #Patch Set 4 : format #Messages
Total messages: 18 (7 generated)
jschuh@chromium.org changed reviewers: + brucedawson@chromium.org
This is a mostly mechanical change that alters the tests so that I can change the internal implementation of CheckedNumeric without the tests breaking. Once I'm done I should be able to support clang/gcc overflow detecting intrinsics.
https://codereview.chromium.org/2003693002/diff/1/base/numerics/safe_numerics... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2003693002/diff/1/base/numerics/safe_numerics... base/numerics/safe_numerics_unittest.cc:66: EXPECT_EQ(expected, CheckedNumeric<Dst>(actual).IsValid()) \ Now that expected has only two values it might make more sense to remove it and have two simpler macros: TEST_EXPECTED_SUCCESS and TEST_EXPECTED_FAILURE. I think this could make the code and the output more readable.
https://codereview.chromium.org/2003693002/diff/1/base/numerics/safe_numerics... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2003693002/diff/1/base/numerics/safe_numerics... base/numerics/safe_numerics_unittest.cc:66: EXPECT_EQ(expected, CheckedNumeric<Dst>(actual).IsValid()) \ On 2016/05/20 21:00:57, brucedawson wrote: > Now that expected has only two values it might make more sense to remove it and > have two simpler macros: > > TEST_EXPECTED_SUCCESS and TEST_EXPECTED_FAILURE. I think this could make the > code and the output more readable. Done.
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/patch-status/2003693002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2003693002/20001
https://codereview.chromium.org/2003693002/diff/20001/base/numerics/safe_nume... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2003693002/diff/20001/base/numerics/safe_nume... base/numerics/safe_numerics_unittest.cc:94: TEST_EXPECTED_VALIDITY( Your search/replace missed a bunch of these, that should use the new macros. The old macro should only be used twice in the final version - to implement the new macros, and nothing else.
Regex search and replace in MSVS is a damn lie!!! https://codereview.chromium.org/2003693002/diff/20001/base/numerics/safe_nume... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2003693002/diff/20001/base/numerics/safe_nume... base/numerics/safe_numerics_unittest.cc:94: TEST_EXPECTED_VALIDITY( On 2016/05/20 21:28:58, brucedawson wrote: > Your search/replace missed a bunch of these, that should use the new macros. The > old macro should only be used twice in the final version - to implement the new > macros, and nothing else. Done.
Description was changed from ========== Simplify validity detection in SafeNumerics tests Intrinics don't identify overflow direction, and we're not relying on it anyway. So, it's easiest to remove it piece by piece. BUG=613003 ========== to ========== Simplify validity detection in SafeNumerics tests Intrinsics don't identify overflow direction, and we're not relying on it anyway. So, it's easiest to remove it piece by piece. BUG=613003 ==========
Needs a run through git cl format, but after that if the tests pass then lgtm.
On 2016/05/20 23:43:43, brucedawson wrote: > Needs a run through git cl format, but after that if the tests pass then lgtm. Well now it's just embarrassing.
The CQ bit was checked by jschuh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/2003693002/#ps60001 (title: "format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2003693002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2003693002/60001
Message was sent while issue was closed.
Description was changed from ========== Simplify validity detection in SafeNumerics tests Intrinsics don't identify overflow direction, and we're not relying on it anyway. So, it's easiest to remove it piece by piece. BUG=613003 ========== to ========== Simplify validity detection in SafeNumerics tests Intrinsics don't identify overflow direction, and we're not relying on it anyway. So, it's easiest to remove it piece by piece. BUG=613003 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Simplify validity detection in SafeNumerics tests Intrinsics don't identify overflow direction, and we're not relying on it anyway. So, it's easiest to remove it piece by piece. BUG=613003 ========== to ========== Simplify validity detection in SafeNumerics tests Intrinsics don't identify overflow direction, and we're not relying on it anyway. So, it's easiest to remove it piece by piece. BUG=613003 Committed: https://crrev.com/819c82686cc9d3ffb14db3f9926f92dcb50ab910 Cr-Commit-Position: refs/heads/master@{#395224} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/819c82686cc9d3ffb14db3f9926f92dcb50ab910 Cr-Commit-Position: refs/heads/master@{#395224} |
