Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2578613002:
Support saturation overrides in saturated_cast (Closed)
DescriptionSupport saturation overrides in saturated_cast
This is a requirement for adding a full saturated type implementation.
BUG=672489
NOTRY=true
Committed: https://crrev.com/186c7dbbea5eef852bf0c4277d1da16f56f9156e
Cr-Commit-Position: refs/heads/master@{#439061}
Patch Set 1 #Patch Set 2 : tests and cleanup #Patch Set 3 : plumbed comparisons through #Patch Set 4 : wip: plumbed with bounds #Patch Set 5 : plumbing done #Patch Set 6 : compile fix #
Total comments: 7
Patch Set 7 : comments and test #Patch Set 8 : build fix #
Messages
Total messages: 36 (22 generated)
Description was changed from ========== Support saturation overrides in saturated::cast This is a requirement for adding a full saturated type implementation. BUG=672489 ========== to ========== Support saturation overrides in saturated_cast This is a requirement for adding a full saturated type implementation. BUG=672489 ==========
jschuh@chromium.org changed reviewers: + brucedawson@chromium.org
brucedawson@ - This is basically just a change to an implementation detail, but I did add some extra tests to cover it.
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.
Patchset #6 (id:100001) has been deleted
Patchset #1 (id:1) has been deleted
jschuh@chromium.org changed reviewers: + eae@chromium.org - brucedawson@chromium.org
Actually, eae@, how about I give you the nice holiday gift of a code review? Sorry the comparisons are so ugly. The overwhelming majority of them compile out, but we're deep in the bowels of template land here.
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...
On 2016/12/15 21:02:27, jschuh wrote: > Actually, eae@, how about I give you the nice holiday gift of a code review? My favorite kind of gift, how did you know? > Sorry the comparisons are so ugly. The overwhelming majority of them compile > out, but we're deep in the bowels of template land here. Ah, good old template magic.
LGTM! https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_con... File base/numerics/safe_conversions.h (right): https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_con... base/numerics/safe_conversions.h:109: // saturated_cast<> is analogous to static_cast<> for numeric types, except This is awesome! I'd love to replace the clampTo<T> macros in Blink with this. https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_con... base/numerics/safe_conversions.h:121: return IsGreaterOrEqual<SrcType, Dst>::Test( Am I right in assuming that all these branches compile away with template expansion and compiler optimizations? https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_con... File base/numerics/safe_conversions_impl.h (right): https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_con... base/numerics/safe_conversions_impl.h:237: ConditionalNegate(SafeUnsignedAbs(value) & ~((T(1) << kShift) - T(1)), Nice! https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_num... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:978: EXPECT_EQ(1, (saturated_cast<int, CastTest2>(0U))); This is great in that you test all the edge values. I'd like to see a test for zero and -1 signed values though, just make make sure.
Patchset #7 (id:160001) has been deleted
https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_con... File base/numerics/safe_conversions.h (right): https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_con... base/numerics/safe_conversions.h:109: // saturated_cast<> is analogous to static_cast<> for numeric types, except On 2016/12/15 23:07:46, eae wrote: > This is awesome! I'd love to replace the clampTo<T> macros in Blink with this. Thanks, sgtm. https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_con... base/numerics/safe_conversions.h:121: return IsGreaterOrEqual<SrcType, Dst>::Test( On 2016/12/15 23:07:46, eae wrote: > Am I right in assuming that all these branches compile away with template > expansion and compiler optimizations? Yes. All of the additional craziness is to cover arbitrary custom bounds, but should compile away. I added comments to clarify a bit. https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_num... File base/numerics/safe_numerics_unittest.cc (right): https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_num... base/numerics/safe_numerics_unittest.cc:978: EXPECT_EQ(1, (saturated_cast<int, CastTest2>(0U))); On 2016/12/15 23:07:46, eae wrote: > This is great in that you test all the edge values. I'd like to see a test for > zero and -1 signed values though, just make make sure. I've added it to clarify that it works with the custom handlers, but it is covered with the generic saturated_cast tests above. So, I also relocated these tests up there (which is where they should have been anyway).
The CQ bit was checked by jschuh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/2578613002/#ps180001 (title: "comments and test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/16 00:31:00, jschuh wrote: > https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_con... > File base/numerics/safe_conversions.h (right): > > https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_con... > base/numerics/safe_conversions.h:109: // saturated_cast<> is analogous to > static_cast<> for numeric types, except > On 2016/12/15 23:07:46, eae wrote: > > This is awesome! I'd love to replace the clampTo<T> macros in Blink with this. > > Thanks, sgtm. > > https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_con... > base/numerics/safe_conversions.h:121: return IsGreaterOrEqual<SrcType, > Dst>::Test( > On 2016/12/15 23:07:46, eae wrote: > > Am I right in assuming that all these branches compile away with template > > expansion and compiler optimizations? > > Yes. All of the additional craziness is to cover arbitrary custom bounds, but > should compile away. I added comments to clarify a bit. > > https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_num... > File base/numerics/safe_numerics_unittest.cc (right): > > https://codereview.chromium.org/2578613002/diff/140001/base/numerics/safe_num... > base/numerics/safe_numerics_unittest.cc:978: EXPECT_EQ(1, (saturated_cast<int, > CastTest2>(0U))); > On 2016/12/15 23:07:46, eae wrote: > > This is great in that you test all the edge values. I'd like to see a test for > > zero and -1 signed values though, just make make sure. > > I've added it to clarify that it works with the custom handlers, but it is > covered with the generic saturated_cast tests above. So, I also relocated these > tests up there (which is where they should have been anyway). Makes sense, thanks! (still LGTM for the record)
The CQ bit was unchecked by commit-bot@chromium.org
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_...) 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 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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Support saturation overrides in saturated_cast This is a requirement for adding a full saturated type implementation. BUG=672489 ========== to ========== Support saturation overrides in saturated_cast This is a requirement for adding a full saturated type implementation. BUG=672489 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...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1481873290326570,
"parent_rev": "6c5f4fa2766ebc594f0178ef5040482253b0a90b", "commit_rev":
"9b6a8e011738f6083cd90cbad37a3b1840affe3c"}
Message was sent while issue was closed.
Description was changed from ========== Support saturation overrides in saturated_cast This is a requirement for adding a full saturated type implementation. BUG=672489 NOTRY=true ========== to ========== Support saturation overrides in saturated_cast This is a requirement for adding a full saturated type implementation. BUG=672489 NOTRY=true Review-Url: https://codereview.chromium.org/2578613002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Support saturation overrides in saturated_cast This is a requirement for adding a full saturated type implementation. BUG=672489 NOTRY=true Review-Url: https://codereview.chromium.org/2578613002 ========== to ========== Support saturation overrides in saturated_cast This is a requirement for adding a full saturated type implementation. BUG=672489 NOTRY=true Committed: https://crrev.com/186c7dbbea5eef852bf0c4277d1da16f56f9156e Cr-Commit-Position: refs/heads/master@{#439061} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/186c7dbbea5eef852bf0c4277d1da16f56f9156e Cr-Commit-Position: refs/heads/master@{#439061}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:180001) has been created in https://codereview.chromium.org/2582063002/ by agrieve@chromium.org. The reason for reverting is: Suspect breaking base_unittests. https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
