Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2502083002:
Restructure ArithmeticPromotion to support alternate promotions (Closed)
DescriptionRestructure ArithmeticPromotion to support alternate promotions
BUG=665584
Committed: https://crrev.com/a1c1b90e415cd5fa064911e1cf4ac70ac8cb20a4
Cr-Commit-Position: refs/heads/master@{#432408}
Patch Set 1 #Patch Set 2 : nit #
Total comments: 4
Messages
Total messages: 20 (7 generated)
jschuh@chromium.org changed reviewers: + tsepez@chromium.org
Okay, this is actually the last CL gating the bitshift patch.
https://codereview.chromium.org/2502083002/diff/20001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/2502083002/diff/20001/base/numerics/safe_math... base/numerics/safe_math.h:240: BASE_NUMERIC_ARITHMETIC_OPERATORS(Mul, *, *=, MAX_EXPONENT_PROMOTION) Shouldn't multiplicaton be BIG_ENOUGH_PROMOTION?
https://codereview.chromium.org/2502083002/diff/20001/base/numerics/safe_math... File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/2502083002/diff/20001/base/numerics/safe_math... base/numerics/safe_math_impl.h:131: LEFT_PROMOTION, // Use the type of the left-hand argument. Do we ever do left/right promotion? Or is this for forthcoming shift?
https://codereview.chromium.org/2502083002/diff/20001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/2502083002/diff/20001/base/numerics/safe_math... base/numerics/safe_math.h:240: BASE_NUMERIC_ARITHMETIC_OPERATORS(Mul, *, *=, MAX_EXPONENT_PROMOTION) On 2016/11/15 22:31:17, Tom Sepez wrote: > Shouldn't multiplicaton be BIG_ENOUGH_PROMOTION? It is, but in the intermediate representation for the CheckedMul function. https://codereview.chromium.org/2502083002/diff/20001/base/numerics/safe_math... File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/2502083002/diff/20001/base/numerics/safe_math... base/numerics/safe_math_impl.h:131: LEFT_PROMOTION, // Use the type of the left-hand argument. On 2016/11/15 22:32:13, Tom Sepez wrote: > Do we ever do left/right promotion? Or is this for forthcoming shift? Just for the forthcoming shift.
LGTM, but still got red bots.
On 2016/11/15 22:51:45, Tom Sepez wrote: > LGTM, but still got red bots. Failed to apply the patch. I assume it's unrelated.
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Restructure ArithmeticPromotion to support alternate promotions BUG=665584 ========== to ========== Restructure ArithmeticPromotion to support alternate promotions BUG=665584 Committed: https://crrev.com/a1c1b90e415cd5fa064911e1cf4ac70ac8cb20a4 Cr-Commit-Position: refs/heads/master@{#432408} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a1c1b90e415cd5fa064911e1cf4ac70ac8cb20a4 Cr-Commit-Position: refs/heads/master@{#432408} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
