|
|
Created:
4 years, 7 months ago by Jeffrey Yasskin Modified:
4 years, 7 months ago Reviewers:
asanka, Peter Kasting, jschuh, danakj, Nico, bsimonnet, Reilly Grant (use Gerrit), dewittj CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow constexpr variables of TimeDelta type.
This allows TimeDelta variables to be global per https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables since they don't have to run a dynamic initializer anymore.
Committed: https://crrev.com/bad4e03c815490e4eb8ceef9b5143b58c6fefb81
Cr-Commit-Position: refs/heads/master@{#394527}
Patch Set 1 #Patch Set 2 : Use static_assert instead of constexpr variables to check for constant-expression-ness. #Patch Set 3 : Remove an unused const variable. #Patch Set 4 : Remove unused constants from a second file. #
Total comments: 3
Patch Set 5 : Remove more unused constants. #
Total comments: 2
Patch Set 6 : Fix overflow from doubles, and test all the boundaries better. #
Total comments: 6
Patch Set 7 : Android doesn't define nexttoward; use nextafter instead. #Patch Set 8 : Undo FromDouble fix. #
Total comments: 2
Patch Set 9 : Generalize FromProduct a bit. #
Total comments: 1
Messages
Total messages: 61 (23 generated)
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976703005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976703005/1
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976703005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976703005/40001
Description was changed from ========== Allow constexpr variables of TimeDelta type. ========== to ========== Allow constexpr variables of TimeDelta type. This allows TimeDelta variables to be global per https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables since they don't have to run a dynamic initializer anymore. ==========
jyasskin@chromium.org changed reviewers: + danakj@chromium.org, thakis@chromium.org
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976703005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976703005/60001
jyasskin@chromium.org changed reviewers: + jschuh@chromium.org
Dana & Nico, as you can see, folks were already creating global TimeDelta variables that would be initialized dynamically. How's this change to make that safe? Justin, could you sanity-check my overflow check? It wound up being too complicated to constexpr'ify all of CheckedNumeric to support TimeDelta's operator*. https://codereview.chromium.org/1976703005/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1976703005/diff/60001/base/time/time.h#newcod... base/time/time.h:607: return FromProduct(secs, Time::kMicrosecondsPerSecond); I had to make this change to avoid making CheckedNumeric constexpr, which is far more complicated. https://codereview.chromium.org/1976703005/diff/60001/base/time/time.h#newcod... base/time/time.h:642: return value > std::numeric_limits<int64_t>::max() / microseconds_per It'd be possible to avoid duplicating the std::numeric_limits<int64_t>::max() / microseconds_per expression by making FromProduct a template with a defaulted second argument, but it seemed less readable that way. https://codereview.chromium.org/1976703005/diff/60001/components/offline_page... File components/offline_pages/offline_page_model.cc (left): https://codereview.chromium.org/1976703005/diff/60001/components/offline_page... components/offline_pages/offline_page_model.cc:46: const base::TimeDelta kPageCleanUpThreshold = base::TimeDelta::FromDays(30); Apparently (https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bu...) making TimeDelta a literal type makes -Wunused-const-variable stronger.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976703005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976703005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1976703005/diff/80001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1976703005/diff/80001/base/time/time.h#newcod... base/time/time.h:632: return value > std::numeric_limits<int64_t>::max() I think this runs afoul of the NarrowingRange corner case: https://code.google.com/p/chromium/codesearch#chromium/src/base/numerics/safe... You could just use base::IsValueInRangeForNumericType (which is a constexpr) and then cap it based on the sign. Or, I could do some evil hackery to make base::satured_cast work as a constexpr.
I'm all good with making things constexpr where they can. How come the static/global variables aren't also becoming constexpr?
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976703005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976703005/100001
On 2016/05/16 18:32:51, danakj wrote: > I'm all good with making things constexpr where they can. How come the > static/global variables aren't also becoming constexpr? A non-const static variable is safe (is initialized by the linker before dynamic initialization runs and isn't destroyed after main()) as long as its type is a literal type, meaning it has a constexpr constructor and a trivial destructor. There's not much benefit from changing even the const global TimeDelta variables to constexpr because we're not passing them to template functions. https://codereview.chromium.org/1976703005/diff/80001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1976703005/diff/80001/base/time/time.h#newcod... base/time/time.h:632: return value > std::numeric_limits<int64_t>::max() On 2016/05/16 16:54:41, jschuh (very slow) wrote: > I think this runs afoul of the NarrowingRange corner case: > https://code.google.com/p/chromium/codesearch#chromium/src/base/numerics/safe... > > You could just use base::IsValueInRangeForNumericType (which is a constexpr) and > then cap it based on the sign. Or, I could do some evil hackery to make > base::satured_cast work as a constexpr. Unless I made a mistake, the FromDouble logic is the same as what already existed. That said, you're correct, and I've tested and fixed the problem. It wasn't a trivial use of IsValueInRangeForNumericType or saturated_cast<> because TimeDelta(numeric_limits<int64_t>::min()) is actually an invalid value, being less than -Max(). https://codereview.chromium.org/1976703005/diff/100001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1976703005/diff/100001/base/time/time.h#newco... base/time/time.h:627: // Note that TimeDelta::FromMicroseconds(numeric_limits<int64_t>::min()) is The fact that this function is happy to produce values less than the minimum might indicate that FromDouble() should just use saturated_cast and accept that TimeDelta::FromMillisecondsD(-pow(2, 63)) < -TimeDelta::Max(). What do the owners think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976703005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976703005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1976703005/diff/100001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1976703005/diff/100001/base/time/time.h#newco... base/time/time.h:627: // Note that TimeDelta::FromMicroseconds(numeric_limits<int64_t>::min()) is On 2016/05/16 18:59:17, Jeffrey Yasskin wrote: > The fact that this function is happy to produce values less than the minimum > might indicate that FromDouble() should just use saturated_cast and accept that > TimeDelta::FromMillisecondsD(-pow(2, 63)) < -TimeDelta::Max(). What do the > owners think? Hm this is rather awkward. FromDouble appears to be the only use of -Max() in the existing codebase outside of tests. I took a look for other callers using Max() in weird ways and I don't see places that depend on FromDouble returning -Max(). Which led me to wonder if it shouldn't return numeric_limits::min() instead of -max? (Though, again we should probably change behaviour independently of making things constexpr.) https://codereview.chromium.org/1976703005/diff/100001/base/time/time.h#newco... base/time/time.h:653: return value > std::numeric_limits<int64_t>::max() / microseconds_per Why do we want to make bad multiplications so well-behaved here? Is this related to constexpr in some way I'm missing? If not I think we should do constexpr without changing behaviour and if we want to fix overflow do that separately.
https://codereview.chromium.org/1976703005/diff/100001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1976703005/diff/100001/base/time/time.h#newco... base/time/time.h:627: // Note that TimeDelta::FromMicroseconds(numeric_limits<int64_t>::min()) is On 2016/05/17 20:28:29, danakj wrote: > On 2016/05/16 18:59:17, Jeffrey Yasskin wrote: > > The fact that this function is happy to produce values less than the minimum > > might indicate that FromDouble() should just use saturated_cast and accept > that > > TimeDelta::FromMillisecondsD(-pow(2, 63)) < -TimeDelta::Max(). What do the > > owners think? > > Hm this is rather awkward. FromDouble appears to be the only use of -Max() in > the existing codebase outside of tests. I took a look for other callers using > Max() in weird ways and I don't see places that depend on FromDouble returning > -Max(). > > Which led me to wonder if it shouldn't return numeric_limits::min() instead of > -max? (Though, again we should probably change behaviour independently of making > things constexpr.) The other use is in FromCheckedNumeric() but spells it -std::numeric_limits<int64_t>::max(). I don't have an opinion on the right behavior, but would like to change that in a dedicated CL so the constexpr stuff doesn't get rolled back if it introduces a bug. https://codereview.chromium.org/1976703005/diff/100001/base/time/time.h#newco... base/time/time.h:653: return value > std::numeric_limits<int64_t>::max() / microseconds_per On 2016/05/17 20:28:29, danakj wrote: > Why do we want to make bad multiplications so well-behaved here? Is this related > to constexpr in some way I'm missing? If not I think we should do constexpr > without changing behaviour and if we want to fix overflow do that separately. The old behavior of FromSeconds() and FromMilliseconds() used CheckedNumeric to catch overflow and replace it with Max() or -Max(): https://code.google.com/p/chromium/codesearch/#chromium/src/base/time/time.cc.... Since CheckedNumeric isn't constexpr, to maintain the behavior, I reproduced the relevant part here. I could split out the improvement to FromDouble if you like. That's not related to preserving the old behavior.
https://codereview.chromium.org/1976703005/diff/100001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1976703005/diff/100001/base/time/time.h#newco... base/time/time.h:653: return value > std::numeric_limits<int64_t>::max() / microseconds_per On 2016/05/17 20:48:51, Jeffrey Yasskin wrote: > On 2016/05/17 20:28:29, danakj wrote: > > Why do we want to make bad multiplications so well-behaved here? Is this > related > > to constexpr in some way I'm missing? If not I think we should do constexpr > > without changing behaviour and if we want to fix overflow do that separately. > > The old behavior of FromSeconds() and FromMilliseconds() used CheckedNumeric to > catch overflow and replace it with Max() or -Max(): > https://code.google.com/p/chromium/codesearch/#chromium/src/base/time/time.cc.... > Since CheckedNumeric isn't constexpr, to maintain the behavior, I reproduced the > relevant part here. Ah, operator* was doing that. I see, and the other times we were using a cast from double to int and comparing, and things are bad. > I could split out the improvement to FromDouble if you like. That's not related > to preserving the old behavior. I think it's a bit better as you say to change behaviour separately in case it causes a problem, so just do the wrong thing with cast in FromDouble, and put a TODO and make another CL to make things better. The question is what's better. I don't like that we're doing this limit checking in 2 different ways, in very different places. We could roll all our own math here in one place, using Max() always instead of numeric_limits? But writing this math is also not awesome. Using saturated_cast consistently seems like the nicest of options, for all of the cases, addition/subtraction/multiplication. We probably want to expose Min() then for people to use if they want what -Max() would have done before. There's a comment in time.cc about why we use max/-max instead of max/min, but I don't really parse it to understand why it wouldn't be feasible. So.. tl;dr I think my POV is: 1. Leave behaviour exactly the same even tho it's bad. FromProduct here is fine. Put some TODOs to delete FromProduct, saturated math, and fix FromDouble. 2. Use saturated cast instead of doing this type of math ourselves. Delete the checked math stuff and FromProduct.
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976703005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976703005/140001
On 2016/05/17 21:17:36, danakj wrote: > https://codereview.chromium.org/1976703005/diff/100001/base/time/time.h > File base/time/time.h (right): > > https://codereview.chromium.org/1976703005/diff/100001/base/time/time.h#newco... > base/time/time.h:653: return value > std::numeric_limits<int64_t>::max() / > microseconds_per > On 2016/05/17 20:48:51, Jeffrey Yasskin wrote: > > On 2016/05/17 20:28:29, danakj wrote: > > > Why do we want to make bad multiplications so well-behaved here? Is this > > related > > > to constexpr in some way I'm missing? If not I think we should do constexpr > > > without changing behaviour and if we want to fix overflow do that > separately. > > > > The old behavior of FromSeconds() and FromMilliseconds() used CheckedNumeric > to > > catch overflow and replace it with Max() or -Max(): > > > https://code.google.com/p/chromium/codesearch/#chromium/src/base/time/time.cc.... > > Since CheckedNumeric isn't constexpr, to maintain the behavior, I reproduced > the > > relevant part here. > > Ah, operator* was doing that. I see, and the other times we were using a cast > from double to int and comparing, and things are bad. > > > I could split out the improvement to FromDouble if you like. That's not > related > > to preserving the old behavior. > > I think it's a bit better as you say to change behaviour separately in case it > causes a problem, so just do the wrong thing with cast in FromDouble, and put a > TODO and make another CL to make things better. > > The question is what's better. I don't like that we're doing this limit checking > in 2 different ways, in very different places. We could roll all our own math > here in one place, using Max() always instead of numeric_limits? But writing > this math is also not awesome. > > Using saturated_cast consistently seems like the nicest of options, for all of > the cases, addition/subtraction/multiplication. We probably want to expose Min() > then for people to use if they want what -Max() would have done before. There's > a comment in time.cc about why we use max/-max instead of max/min, but I don't > really parse it to understand why it wouldn't be feasible. > > So.. tl;dr I think my POV is: > > 1. Leave behaviour exactly the same even tho it's bad. FromProduct here is fine. > Put some TODOs to delete FromProduct, saturated math, and fix FromDouble. > 2. Use saturated cast instead of doing this type of math ourselves. Delete the > checked math stuff and FromProduct. Done. I'm not sure we can replace the use of CheckedNumeric with just saturated_cast, because we have to avoid certain arithmetic, not just cast back from a larger type after doing the arithmetic. But we can figure that out in the next CL.
On 2016/05/16 18:59:17, Jeffrey Yasskin wrote: > On 2016/05/16 18:32:51, danakj wrote: > > I'm all good with making things constexpr where they can. How come the > > static/global variables aren't also becoming constexpr? > > A non-const static variable is safe (is initialized by the linker before dynamic > initialization runs and isn't destroyed after main()) as long as its type is a > literal type, meaning it has a constexpr constructor and a trivial destructor. > > There's not much benefit from changing even the const global TimeDelta variables > to constexpr because we're not passing them to template functions. I guess I like the variables being constexpr cuz it documents and enforces that they are not and will not become static initializers. Does that make sense tho? base/ LGTM, one naming nit. https://codereview.chromium.org/1976703005/diff/140001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1976703005/diff/140001/base/time/time.h#newco... base/time/time.h:643: int microseconds_per) { nit: maybe not microseconds_per, the microseconds_per-ness is a detail of the call sites. A more general name would be nice. Maybe even v1, v2 or something.. Also maybe it should be an int64_t?
https://codereview.chromium.org/1976703005/diff/140001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1976703005/diff/140001/base/time/time.h#newco... base/time/time.h:643: int microseconds_per) { On 2016/05/17 21:53:14, danakj wrote: > nit: maybe not microseconds_per, the microseconds_per-ness is a detail of the > call sites. A more general name would be nice. Maybe even v1, v2 or something.. > Also maybe it should be an int64_t? This code gets to be simpler than CheckedNumeric specifically because its second parameter can only take the positive values that are actually passed in. It's not a fully generic function that can be used anywhere like operator* is. So, I can use "value" and "positive_value"?
The CQ bit was checked by jyasskin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1976703005/#ps160001 (title: "Generalize FromProduct a bit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976703005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976703005/160001
On Tue, May 17, 2016 at 3:10 PM, <jyasskin@chromium.org> wrote: > > https://codereview.chromium.org/1976703005/diff/140001/base/time/time.h > File base/time/time.h (right): > > > https://codereview.chromium.org/1976703005/diff/140001/base/time/time.h#newco... > base/time/time.h:643: int microseconds_per) { > On 2016/05/17 21:53:14, danakj wrote: > > nit: maybe not microseconds_per, the microseconds_per-ness is a detail > of the > > call sites. A more general name would be nice. Maybe even v1, v2 or > something.. > > Also maybe it should be an int64_t? > > This code gets to be simpler than CheckedNumeric specifically because > its second parameter can only take the positive values that are actually > passed in. It's not a fully generic function that can be used anywhere > like operator* is. So, I can use "value" and "positive_value"? > cool ya > https://codereview.chromium.org/1976703005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by jyasskin@chromium.org
jyasskin@chromium.org changed reviewers: + achaulk@chromium.org, asanka@chromium.org, dewittj@chromium.org, reillyg@chromium.org
Please review a couple deletions of unused code: achaulk: components/feedback/feedback_data_unittest.cc dewittj: components/offline_pages/offline_page_model.cc asanka: content/browser/download/base_file_unittest.cc reillyg: extensions/browser/quota_service_unittest.cc
lgtm
lgtm
lgtm
lgtm
lgtm
jyasskin@chromium.org changed reviewers: + bsimonnet@chromium.org - achaulk@chromium.org
s/achaulk/bsimonnet/ for components/feedback/feedback_data_unittest.cc
s/achaulk/bsimonnet/ for components/feedback/feedback_data_unittest.cc
lgtm
The CQ bit was checked by jyasskin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976703005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976703005/160001
Message was sent while issue was closed.
Description was changed from ========== Allow constexpr variables of TimeDelta type. This allows TimeDelta variables to be global per https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables since they don't have to run a dynamic initializer anymore. ========== to ========== Allow constexpr variables of TimeDelta type. This allows TimeDelta variables to be global per https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables since they don't have to run a dynamic initializer anymore. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Allow constexpr variables of TimeDelta type. This allows TimeDelta variables to be global per https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables since they don't have to run a dynamic initializer anymore. ========== to ========== Allow constexpr variables of TimeDelta type. This allows TimeDelta variables to be global per https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables since they don't have to run a dynamic initializer anymore. Committed: https://crrev.com/bad4e03c815490e4eb8ceef9b5143b58c6fefb81 Cr-Commit-Position: refs/heads/master@{#394527} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/bad4e03c815490e4eb8ceef9b5143b58c6fefb81 Cr-Commit-Position: refs/heads/master@{#394527}
Message was sent while issue was closed.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1976703005/diff/160001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1976703005/diff/160001/base/time/time.h#newco... base/time/time.h:585: constexpr inline TimeDelta TimeDelta::FromDays(int days) { Doesn't constexpr imply the same rules (e.g. for ODR) as "inline"? Shouldn't we remove "inline" on these?
Message was sent while issue was closed.
On 2016/05/18 20:53:28, Peter Kasting wrote: > https://codereview.chromium.org/1976703005/diff/160001/base/time/time.h > File base/time/time.h (right): > > https://codereview.chromium.org/1976703005/diff/160001/base/time/time.h#newco... > base/time/time.h:585: constexpr inline TimeDelta TimeDelta::FromDays(int days) { > Doesn't constexpr imply the same rules (e.g. for ODR) as "inline"? Shouldn't we > remove "inline" on these? Good question. I don't know if 'constexpr' implies 'inline'. Do you happen to have a reference for that, or should I try to find it myself?
Message was sent while issue was closed.
On 2016/05/18 20:56:24, Jeffrey Yasskin wrote: > On 2016/05/18 20:53:28, Peter Kasting wrote: > > https://codereview.chromium.org/1976703005/diff/160001/base/time/time.h > > File base/time/time.h (right): > > > > > https://codereview.chromium.org/1976703005/diff/160001/base/time/time.h#newco... > > base/time/time.h:585: constexpr inline TimeDelta TimeDelta::FromDays(int days) > { > > Doesn't constexpr imply the same rules (e.g. for ODR) as "inline"? Shouldn't > we > > remove "inline" on these? > > Good question. I don't know if 'constexpr' implies 'inline'. Do you happen to > have a reference for that, or should I try to find it myself? Yep: [dcl.constexpr]p2. I'll send a fix. Thanks. |