base/numerics cleanup 2/n: More modern techniques [chromium/src : main]
0 views
Skip to first unread message
Peter Kasting (Gerrit)
Nov 12, 2024, 2:42:30 PMNov 12
to Stefan Zager, Stephen Chenney, Dirk Schulze, AyeAye, findit...@appspot.gserviceaccount.com, Tom Sepez, Tricium, Chromium LUCI CQ, chromium...@chromium.org, blink-reviews-p...@chromium.org, kinuko...@chromium.org, fmalit...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fserb...@chromium.org
Attention needed from Stefan Zager and Tom Sepez
Peter Kasting voted and added 1 comment![]( "Open in Gerrit")
Votes added by Peter Kasting
| Auto-Submit | +0 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Stefan Zager
- Tom Sepez
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Nov 12, 2024, 3:56:59 PMNov 12
to Peter Kasting, findit...@appspot.gserviceaccount.com, Stefan Zager, Stephen Chenney, Dirk Schulze, AyeAye, Tricium, Chromium LUCI CQ, chromium...@chromium.org, blink-reviews-p...@chromium.org, kinuko...@chromium.org, fmalit...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fserb...@chromium.org
Attention needed from Peter Kasting and Stefan Zager
Tom Sepez voted and added 4 comments![]( "Open in Gerrit")
Votes added by Tom Sepez
| Code-Review | +1 |
4 comments
File base/numerics/checked_math_impl.h
Line 538, Patchset 4 (Latest):
// Written this way because std::isfinite is not reliably constexpr.Tom Sepez . unresolved
nit: maybe mention that it might be in C++23 ?
File base/numerics/clamped_math_impl.h
Line 120, Patchset 4 (Latest):
if (!std::is_constant_evaluated() && ClampedMulFastOp<T, U>::is_supported) {Tom Sepez . unresolved
can this be constexpr-if ?
File base/safe_numerics_unittest.cc
Line 1896, Patchset 4 (Parent):
void TestWrappingMathSigned() {Tom Sepez . resolved
Nice, how this folds into one overloaded form.
File third_party/blink/renderer/platform/geometry/layout_unit.h
Line 358, Patchset 4 (Latest):
if (std::is_constant_evaluated() || sizeof(Storage) > sizeof(int)) {Tom Sepez . unresolved
constexpr-if ? and below.
Related details
Attention is currently required from:
- Peter Kasting
- Stefan Zager
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Nov 12, 2024, 3:57:31 PMNov 12
to Peter Kasting, findit...@appspot.gserviceaccount.com, Stefan Zager, Stephen Chenney, Dirk Schulze, AyeAye, Tricium, Chromium LUCI CQ, chromium...@chromium.org, blink-reviews-p...@chromium.org, kinuko...@chromium.org, fmalit...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fserb...@chromium.org
Attention needed from Peter Kasting and Stefan Zager
Tom Sepez added 1 comment![]( "Open in Gerrit")
File base/numerics/checked_math_impl.h
Line 538, Patchset 4 (Latest):
// Written this way because std::isfinite is not reliably constexpr.Tom Sepez . unresolved
nit: maybe mention that it might be in C++23 ?
Tom Sepez
or rather, that it will be if I'm reading cppreference correctly.
Stefan Zager (Gerrit)
Nov 12, 2024, 4:20:07 PMNov 12
to Peter Kasting, Tom Sepez, findit...@appspot.gserviceaccount.com, Stephen Chenney, Dirk Schulze, AyeAye, Tricium, Chromium LUCI CQ, chromium...@chromium.org, blink-reviews-p...@chromium.org, kinuko...@chromium.org, fmalit...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fserb...@chromium.org
Attention needed from Peter Kasting
Stefan Zager voted and added 1 comment![]( "Open in Gerrit")
Related details
Attention is currently required from:
- Peter Kasting
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Kasting (Gerrit)
Nov 12, 2024, 6:50:26 PMNov 12
to Stefan Zager, Tom Sepez, findit...@appspot.gserviceaccount.com, Stephen Chenney, Dirk Schulze, AyeAye, Tricium, Chromium LUCI CQ, chromium...@chromium.org, blink-reviews-p...@chromium.org, kinuko...@chromium.org, fmalit...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fserb...@chromium.org
Peter Kasting added 3 comments![]( "Open in Gerrit")
File base/numerics/checked_math_impl.h
Line 538, Patchset 4:
// Written this way because std::isfinite is not reliably constexpr.Tom Sepez . resolved
Tom Sepeznit: maybe mention that it might be in C++23 ?
or rather, that it will be if I'm reading cppreference correctly.
Peter Kasting
Done
File base/numerics/clamped_math_impl.h
Line 120, Patchset 4:
if (!std::is_constant_evaluated() && ClampedMulFastOp<T, U>::is_supported) {Tom Sepez . resolved
can this be constexpr-if ?
Peter Kasting
It cannot; if you force the conditional to be compile-time-evaluated like that, then `is_constant_evaluated()` is always true, so the negative branch becomes dead, and the compiler yells at you.
This is sorta weird and unintuitive, I had to test it.
File third_party/blink/renderer/platform/geometry/layout_unit.h
Line 358, Patchset 4:
if (std::is_constant_evaluated() || sizeof(Storage) > sizeof(int)) {Tom Sepez . resolved
Peter Kastingconstexpr-if ? and below.
Acknowledged
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Nov 12, 2024, 8:23:00 PMNov 12
to Peter Kasting, Stefan Zager, Tom Sepez, findit...@appspot.gserviceaccount.com, Stephen Chenney, Dirk Schulze, AyeAye, Tricium, chromium...@chromium.org, blink-reviews-p...@chromium.org, kinuko...@chromium.org, fmalit...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fserb...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Commit message:
base/numerics cleanup 2/n: More modern techniques
* Use `std::is_constant_evaluated()`
* Use `_t` versions of type traits
* Use concepts more when available
* Convert more asserts to constraints
* const -> constexpr more
* if -> constexpr if more
* =default and assign-in-declaration
* `std::conditional_t<>` instead of specializations more
* Rely on C++20 rewritten comparison ops
* assert message is optional
Also removes a few pieces of dead code.
Bug: none
Change-Id: Ifd3dbd5e4d0713630ed20ddad41f117ba963a354
Reviewed-by: Stefan Zager <sza...@chromium.org>
Commit-Queue: Peter Kasting <pkas...@chromium.org>
Reviewed-by: Tom Sepez <tse...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1382091}
Files:
- M base/numerics/checked_math.h
- M base/numerics/checked_math_impl.h
- M base/numerics/clamped_math.h
- M base/numerics/clamped_math_impl.h
- M base/numerics/safe_conversions.h
- M base/numerics/safe_conversions_impl.h
- M base/safe_numerics_unittest.cc
- M third_party/blink/renderer/platform/geometry/layout_unit.h
Change size: L
Delta: 8 files changed, 269 insertions(+), 350 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Coverage: +1 by findit...@appspot.gserviceaccount.com
Code-Review: +1 by Tom Sepez, +1 by Stefan Zager
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages