Add IsValidAnd() and IsInvalidOr() helpers to CheckedNumeric [chromium/src : main]
David Benjamin (Gerrit)
David Benjamin added 2 comments![]( "Open in Gerrit")
// `predicate`'s single parameter.Not a strong feeling either way, just an idle thought: the conversion case seems like this is really two values in one: `Cast<OtherType>()` and `IsValidAnd`.
Would it make sense to make this specifically take the underlying type, and then explicitly call `Cast<uint16_t>().IsValidAnd(...)` if folks want the other type?
EXPECT_FALSE(invalid.IsValidAnd(IntGreaterThanZero));
EXPECT_FALSE(invalid.IsValidAnd(IntLessThanZero));
EXPECT_FALSE(invalid.IsValidAnd(Int64GreaterThanZero));
EXPECT_FALSE(invalid.IsValidAnd(Int64LessThanZero));
EXPECT_FALSE(invalid.IsValidAnd(Int16GreaterThanZero));
EXPECT_FALSE(invalid.IsValidAnd(Int16LessThanZero));
David Benjamingiven that I have faith that lambda captures work correctly, do we need to test all these named functions? Alternatively, do we just test the named functions?
There also aren't any lambda captures involved anyway. 😊
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 3 comments![]( "Open in Gerrit")
// `predicate`'s single parameter.Not a strong feeling either way, just an idle thought: the conversion case seems like this is really two values in one: `Cast<OtherType>()` and `IsValidAnd`.
Would it make sense to make this specifically take the underlying type, and then explicitly call `Cast<uint16_t>().IsValidAnd(...)` if folks want the other type?
I don't love making people write out types twice, even if they're relatively short in this case. The problem (to me) is it's not always clear what the result of `base::CheckAdd(...)` will be, especially if the two arms differ in types.
Other possibilities are to allow this API to work without an explicit argument if the types match exactly and requirement an explicit type argument otherwise. Always requiring an exact match would be a bit cumbersome IMO. But... it's a bit clunky either way.
I settled with this, but it is definitely a little magical. I'll link this out to see if there are other general opinions.
static bool IntGreaterThanZero(int value) {
return value > 0;
}
static bool IntLessThanZero(int value) {
return value < 0;
}
static bool Int64GreaterThanZero(int64_t value) {
return value > 0;
}
static bool Int64LessThanZero(int64_t value) {
return value < 0;
}
static bool Int16GreaterThanZero(int16_t value) {
return value > 0;
}
static bool Int16LessThanZero(int16_t value) {
return value < 0;
}
Daniel ChengMaybe these should have side-effects, so we can check if they were short-circuited. Or gmock matchers.
Done
EXPECT_FALSE(invalid.IsValidAnd(IntGreaterThanZero));
EXPECT_FALSE(invalid.IsValidAnd(IntLessThanZero));
EXPECT_FALSE(invalid.IsValidAnd(Int64GreaterThanZero));
EXPECT_FALSE(invalid.IsValidAnd(Int64LessThanZero));
EXPECT_FALSE(invalid.IsValidAnd(Int16GreaterThanZero));
EXPECT_FALSE(invalid.IsValidAnd(Int16LessThanZero));
David Benjamingiven that I have faith that lambda captures work correctly, do we need to test all these named functions? Alternatively, do we just test the named functions?
There also aren't any lambda captures involved anyway. 😊
Well, now there are 😊
It's mostly to test the traits work, but then it looks kind of weird if we only have some of the cases IMO. I will concede it looks very verbose.
Related details
- David Benjamin
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
static bool* did_call_predicate = nullptr;bleh on mutable global storage. Maybe only check the short-circuiting via lambda capture by referecne of a local variable.
Related details
- Daniel Cheng
- David Benjamin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
// `predicate`'s single parameter.Daniel ChengNot a strong feeling either way, just an idle thought: the conversion case seems like this is really two values in one: `Cast<OtherType>()` and `IsValidAnd`.
Would it make sense to make this specifically take the underlying type, and then explicitly call `Cast<uint16_t>().IsValidAnd(...)` if folks want the other type?
I don't love making people write out types twice, even if they're relatively short in this case. The problem (to me) is it's not always clear what the result of `base::CheckAdd(...)` will be, especially if the two arms differ in types.
Other possibilities are to allow this API to work without an explicit argument if the types match exactly and requirement an explicit type argument otherwise. Always requiring an exact match would be a bit cumbersome IMO. But... it's a bit clunky either way.
I settled with this, but it is definitely a little magical. I'll link this out to see if there are other general opinions.
Fair enough. (I don't have strong feelings here. Just an idle thought.)
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Commit-Queue | +2 |
3 comments
// `predicate`'s single parameter.Daniel ChengNot a strong feeling either way, just an idle thought: the conversion case seems like this is really two values in one: `Cast<OtherType>()` and `IsValidAnd`.
Would it make sense to make this specifically take the underlying type, and then explicitly call `Cast<uint16_t>().IsValidAnd(...)` if folks want the other type?
David BenjaminI don't love making people write out types twice, even if they're relatively short in this case. The problem (to me) is it's not always clear what the result of `base::CheckAdd(...)` will be, especially if the two arms differ in types.
Other possibilities are to allow this API to work without an explicit argument if the types match exactly and requirement an explicit type argument otherwise. Always requiring an exact match would be a bit cumbersome IMO. But... it's a bit clunky either way.
I settled with this, but it is definitely a little magical. I'll link this out to see if there are other general opinions.
Fair enough. (I don't have strong feelings here. Just an idle thought.)
Acknowledged
static bool* did_call_predicate = nullptr;bleh on mutable global storage. Maybe only check the short-circuiting via lambda capture by referecne of a local variable.
As discussed offline, landing as-is (but for the record, I agree that globals aren't great)
EXPECT_FALSE(invalid.IsValidAnd(IntGreaterThanZero));
EXPECT_FALSE(invalid.IsValidAnd(IntLessThanZero));
EXPECT_FALSE(invalid.IsValidAnd(Int64GreaterThanZero));
EXPECT_FALSE(invalid.IsValidAnd(Int64LessThanZero));
EXPECT_FALSE(invalid.IsValidAnd(Int16GreaterThanZero));
EXPECT_FALSE(invalid.IsValidAnd(Int16LessThanZero));
David Benjamingiven that I have faith that lambda captures work correctly, do we need to test all these named functions? Alternatively, do we just test the named functions?
Daniel ChengThere also aren't any lambda captures involved anyway. 😊
Well, now there are 😊
It's mostly to test the traits work, but then it looks kind of weird if we only have some of the cases IMO. I will concede it looks very verbose.
- David Benjamin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Add IsValidAnd() and IsInvalidOr() helpers to CheckedNumeric
- M base/numerics/checked_math.h
- M base/numerics/checked_math_impl.h
- M base/safe_numerics_nocompile.nc
- M base/safe_numerics_unittest.cc
Code-Review: +1 by Tom Sepez
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |