Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(392)

Issue 131063002: Expand support in safe_numeric.h (Closed)

Created:
6 years, 11 months ago by jschuh
Modified:
6 years, 11 months ago
CC:
chromium-reviews, erikwright+watch_chromium.org, Jeffrey Yasskin
Visibility:
Public.

Description

Expand support in safe_numeric.h * Refactor code in preparation for safe math operations. * Support float type for numeric casts. * Expose IsValueInRangeForNumericType as part of the API. * Add saturated_cast. BUG=332611 R=akalin@chromium.org, willchan@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245183

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Total comments: 4

Patch Set 5 : Removed macros and split out implementation header #

Total comments: 14

Patch Set 6 : Switch to enums and fix style issues #

Patch Set 7 : Fix compile issues #

Patch Set 8 : Work in progress #

Patch Set 9 : WIP: test coverage #

Patch Set 10 : Ready for review #

Total comments: 21

Patch Set 11 : Actioned feedback #

Total comments: 4

Patch Set 12 : final #

Total comments: 4

Patch Set 13 : final: this time it's for real #

Unified diffs Side-by-side diffs Delta from patch set Stats (+465 lines, -256 lines) Patch
M base/base.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +0 lines, -1 line 0 comments Download
M base/base.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M base/safe_numerics.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +37 lines, -110 lines 0 comments Download
A base/safe_numerics_impl.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +183 lines, -0 lines 0 comments Download
M base/safe_numerics_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +244 lines, -116 lines 0 comments Download
M base/safe_numerics_unittest.nc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +0 lines, -29 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
jschuh
This is phase one in the safe integer math. I generalized the range checking a ...
6 years, 11 months ago (2014-01-09 15:56:58 UTC) #1
awong
2 comment nits first. https://codereview.chromium.org/131063002/diff/340001/base/safe_numerics.h File base/safe_numerics.h (right): https://codereview.chromium.org/131063002/diff/340001/base/safe_numerics.h#newcode17 base/safe_numerics.h:17: struct NumericRangeTestImpl; This template needs ...
6 years, 11 months ago (2014-01-09 16:35:03 UTC) #2
jschuh
Big picture, I'm going with what we we discussed in chat about refactoring into traits ...
6 years, 11 months ago (2014-01-09 17:23:45 UTC) #3
jschuh
@akali - awong suggested I have you review this while he's out.
6 years, 11 months ago (2014-01-13 23:05:50 UTC) #4
akalin
initial comments https://codereview.chromium.org/131063002/diff/640001/base/safe_numerics_impl.h File base/safe_numerics_impl.h (right): https://codereview.chromium.org/131063002/diff/640001/base/safe_numerics_impl.h#newcode18 base/safe_numerics_impl.h:18: template<typename Dest, nit: space before < https://codereview.chromium.org/131063002/diff/640001/base/safe_numerics_impl.h#newcode20 ...
6 years, 11 months ago (2014-01-14 00:49:19 UTC) #5
jschuh
Sorry for the large scope of changes in this round. I cleaned up the formatting ...
6 years, 11 months ago (2014-01-14 05:24:32 UTC) #6
jschuh
I had some corner cases that were hard to catch. So, I'm in the process ...
6 years, 11 months ago (2014-01-14 20:18:26 UTC) #7
akalin
On 2014/01/14 20:18:26, Justin Schuh wrote: > I had some corner cases that were hard ...
6 years, 11 months ago (2014-01-14 20:42:19 UTC) #8
jschuh
Okay, should be ready for review. I cleaned up the conversion logic and used enums ...
6 years, 11 months ago (2014-01-15 20:14:14 UTC) #9
jschuh
+willchan Before Albert left on vacation he said I should get Fred to review for ...
6 years, 11 months ago (2014-01-15 21:57:28 UTC) #10
akalin
one more pass https://codereview.chromium.org/131063002/diff/1800001/base/safe_numerics.h File base/safe_numerics.h (right): https://codereview.chromium.org/131063002/diff/1800001/base/safe_numerics.h#newcode36 base/safe_numerics.h:36: // Optimization for floating point values, ...
6 years, 11 months ago (2014-01-16 00:21:54 UTC) #11
willchan no longer on Chromium
I'm going to disappear Friday for a week (HTTP/2 conf in Zurich). And I'm busy ...
6 years, 11 months ago (2014-01-16 00:37:16 UTC) #12
jschuh
https://codereview.chromium.org/131063002/diff/1800001/base/safe_numerics.h File base/safe_numerics.h (right): https://codereview.chromium.org/131063002/diff/1800001/base/safe_numerics.h#newcode36 base/safe_numerics.h:36: // Optimization for floating point values, which already saturate. ...
6 years, 11 months ago (2014-01-16 01:26:30 UTC) #13
akalin
lgtm after nits https://codereview.chromium.org/131063002/diff/1800001/base/safe_numerics.h File base/safe_numerics.h (right): https://codereview.chromium.org/131063002/diff/1800001/base/safe_numerics.h#newcode36 base/safe_numerics.h:36: // Optimization for floating point values, ...
6 years, 11 months ago (2014-01-16 02:28:11 UTC) #14
jschuh
Should be good now. Will, let me know if you feel comfortable rubberstamping this after ...
6 years, 11 months ago (2014-01-16 02:53:35 UTC) #15
willchan no longer on Chromium
/cc jyasskin LGTM rubberstamp Jeffrey, I added you in case you wanted to have fun ...
6 years, 11 months ago (2014-01-16 03:02:06 UTC) #16
jschuh
https://codereview.chromium.org/131063002/diff/2180001/base/safe_numerics.h File base/safe_numerics.h (right): https://codereview.chromium.org/131063002/diff/2180001/base/safe_numerics.h#newcode18 base/safe_numerics.h:18: inline bool IsValueInRangeForNumericType(Src value) { On 2014/01/16 03:02:06, willchan ...
6 years, 11 months ago (2014-01-16 04:08:19 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jschuh@chromium.org/131063002/2150003
6 years, 11 months ago (2014-01-16 04:10:02 UTC) #18
jschuh
6 years, 11 months ago (2014-01-16 06:57:29 UTC) #19
Message was sent while issue was closed.
Committed patchset #13 manually as r245183 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698