Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2191163003:
Add compile time assert to help catch new fields (Closed)
|
Created:
4 years, 4 months ago by Bryan McQuade Modified:
4 years, 1 month ago CC:
chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@fixequal Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd compile time assert to help catch new fields.
We recently encountered a case where a field in PageLoadTiming
wasn't included in the operator== and IsEmpty implementations.
This compile-time assert is intended to make developers aware
that they need to update the implementations of these functions
any time they add a new field to PageLoadTiming.
BUG=632832
Patch Set 1 #Patch Set 2 : fix comment #Patch Set 3 : fix comment #
Total comments: 1
Patch Set 4 : add comment #
Total comments: 4
Patch Set 5 : address kinuko comments #Patch Set 6 : address kinuko comments #Patch Set 7 : remove comment #Messages
Total messages: 36 (28 generated)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add compile time assert to help catch missing fields. BUG= ========== to ========== Add compile time assert to help catch new fields. We recently encountered a case where a field in PageLoadTiming wasn't included in the operator== and IsEmpty implementations. This compile-time assert is intended to make developers aware that they need to update the implementations of these functions any time they add a new field to PageLoadTiming. BUG= ==========
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
https://codereview.chromium.org/2191163003/diff/40001/chrome/common/page_load... File chrome/common/page_load_metrics/page_load_timing.cc (right): https://codereview.chromium.org/2191163003/diff/40001/chrome/common/page_load... chrome/common/page_load_metrics/page_load_timing.cc:29: "Make sure to update IsEmpty and operator== to reflect your changes."); Could we always be sure that we have no padding between fields?
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add compile time assert to help catch new fields. We recently encountered a case where a field in PageLoadTiming wasn't included in the operator== and IsEmpty implementations. This compile-time assert is intended to make developers aware that they need to update the implementations of these functions any time they add a new field to PageLoadTiming. BUG= ========== to ========== Add compile time assert to help catch new fields. We recently encountered a case where a field in PageLoadTiming wasn't included in the operator== and IsEmpty implementations. This compile-time assert is intended to make developers aware that they need to update the implementations of these functions any time they add a new field to PageLoadTiming. BUG=632832 ==========
On 2016/08/01 at 15:38:00, kinuko wrote: > https://codereview.chromium.org/2191163003/diff/40001/chrome/common/page_load... > File chrome/common/page_load_metrics/page_load_timing.cc (right): > > https://codereview.chromium.org/2191163003/diff/40001/chrome/common/page_load... > chrome/common/page_load_metrics/page_load_timing.cc:29: "Make sure to update IsEmpty and operator== to reflect your changes."); > Could we always be sure that we have no padding between fields? Good question. We don't encounter padding for base::Time and base::Optional<base::TimeDelta>, but this may not be generally true for other types. I added a comment about this. If for some reason these structs change in the future such that there is padding, or we add fields to PageLoadTiming that cause us to incur padding, we'll need to update or remove this assertion. For the types we are depending on, I think that is unlikely. base::Time and base::TimeDelta are spec'd to be sizeof(double) - their documentation says they are 64 bits and to always pass them by value. So we're committed to no size changes there, more or less. base::Optional<> also forces the alignment of the optional member, which I believe also makes sure the overall struct is aligned to avoid padding. So for the types we are working with, I feel we are safe. If for some reason that changes in the future, we can revisit this, and if the complexity increases substantially as a result, I'd be open to removing this static_assert. But for now, I think this is safe.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % some nits https://codereview.chromium.org/2191163003/diff/60001/chrome/common/page_load... File chrome/common/page_load_metrics/page_load_timing.cc (right): https://codereview.chromium.org/2191163003/diff/60001/chrome/common/page_load... chrome/common/page_load_metrics/page_load_timing.cc:18: // operator== and IsEmpty below to reflect your changes. nit: "update operator==, IsEmpty and kNumTImeDeltas below to..." nit: now we have similar comments in 3 places (here, in page_load_timing.h and error text for static_assert). Maybe just update the comment in page_load_timing.h to include kNumTimeDeltas, remove this comment but use this text (with big '*** IMPORTANT ***') as the error text for the static_assert? https://codereview.chromium.org/2191163003/diff/60001/chrome/common/page_load... chrome/common/page_load_metrics/page_load_timing.cc:30: kSizeOfTime + kSizeOfAllOptionalTimeDeltas, Using constexpr function might make these const's + static_assert a little prettier? (Kinda preferential, so feel free to ignore if you disagree) constexpr size_t CalculatePageLoadTimingSize() { // This assumes that the members of PageLoadTiming are stored without... return sizeof(base::Time) /* sizeof navigation_start */ + sizeof(base::Optional<base::TimeDelta>) * kNumTimeDeltas; } static_assert(sizeof(PageLoadTiming) == CalculatePageLoadTimingSize(), "...");
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
bmcquade@chromium.org changed reviewers: + csharrison@chromium.org
Thanks! I've addressed these comments. I decided to go forward with this change. csharrison, can you take a look as well? I think this approach ends up being better than putting the fields in a separate header and using macro magic to construct the struct and associated functions. https://codereview.chromium.org/2191163003/diff/60001/chrome/common/page_load... File chrome/common/page_load_metrics/page_load_timing.cc (right): https://codereview.chromium.org/2191163003/diff/60001/chrome/common/page_load... chrome/common/page_load_metrics/page_load_timing.cc:18: // operator== and IsEmpty below to reflect your changes. On 2016/08/02 at 16:09:00, kinuko (slow) wrote: > nit: "update operator==, IsEmpty and kNumTImeDeltas below to..." > > nit: now we have similar comments in 3 places (here, in page_load_timing.h and error text for static_assert). > > Maybe just update the comment in page_load_timing.h to include kNumTimeDeltas, remove this comment but use this text (with big '*** IMPORTANT ***') as the error text for the static_assert? Good idea, thanks! I updated the code to reflect your suggestion. https://codereview.chromium.org/2191163003/diff/60001/chrome/common/page_load... chrome/common/page_load_metrics/page_load_timing.cc:30: kSizeOfTime + kSizeOfAllOptionalTimeDeltas, On 2016/08/02 at 16:09:00, kinuko (slow) wrote: > Using constexpr function might make these const's + static_assert a little prettier? > (Kinda preferential, so feel free to ignore if you disagree) > > constexpr size_t CalculatePageLoadTimingSize() { > // This assumes that the members of PageLoadTiming are stored without... > return sizeof(base::Time) /* sizeof navigation_start */ + > sizeof(base::Optional<base::TimeDelta>) * kNumTimeDeltas; > } > > static_assert(sizeof(PageLoadTiming) == CalculatePageLoadTimingSize(), "..."); Yes, constexpr makes this easier to read. I made this change. Thanks!
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Curious, have you considered using a presubmit here? Even a dead simple warning like "You updated PageLoadTiming, did you update operator==... y/N" might be just as effective as this.
On 2016/09/07 at 15:03:11, csharrison wrote: > Curious, have you considered using a presubmit here? Even a dead simple warning like "You updated PageLoadTiming, did you update operator==... y/N" might be just as effective as this. That's an interesting idea that I hadn't considered - thanks for suggesting it! I think I prefer auditing this directly at compile time, both because the presubmit might be noisy (presumably it would fire on any change to page_load_metrics.h, even if we aren't adding TimeDelta fields - that's a bit confusing), because it's possible to bypass presubmits (hopefully nobody does so, but it's possible), and because presubmits trigger much later in the development process than compile time assertions. Given that this is reasonably simple and self contained in one anonymous namespace block, I think this approach strikes the best overall balance between the various options. What do you think?
On 2016/09/07 15:08:09, Bryan McQuade wrote: > On 2016/09/07 at 15:03:11, csharrison wrote: > > Curious, have you considered using a presubmit here? Even a dead simple > warning like "You updated PageLoadTiming, did you update operator==... y/N" > might be just as effective as this. > > That's an interesting idea that I hadn't considered - thanks for suggesting it! > I think I prefer auditing this directly at compile time, both because the > presubmit might be noisy (presumably it would fire on any change to > page_load_metrics.h, even if we aren't adding TimeDelta fields - that's a bit > confusing), because it's possible to bypass presubmits (hopefully nobody does > so, but it's possible), and because presubmits trigger much later in the > development process than compile time assertions. Given that this is reasonably > simple and self contained in one anonymous namespace block, I think this > approach strikes the best overall balance between the various options. What do > you think? I would personally prefer a presubmit, as this code is confusing and seems fragile if any part of PageLoadTiming internals changes (or the time classes change). However, I don't feel strongly enough to block this, so lgtm if you prefer this way.
On 2016/09/07 at 15:12:44, csharrison wrote: > On 2016/09/07 15:08:09, Bryan McQuade wrote: > > On 2016/09/07 at 15:03:11, csharrison wrote: > > > Curious, have you considered using a presubmit here? Even a dead simple > > warning like "You updated PageLoadTiming, did you update operator==... y/N" > > might be just as effective as this. > > > > That's an interesting idea that I hadn't considered - thanks for suggesting it! > > I think I prefer auditing this directly at compile time, both because the > > presubmit might be noisy (presumably it would fire on any change to > > page_load_metrics.h, even if we aren't adding TimeDelta fields - that's a bit > > confusing), because it's possible to bypass presubmits (hopefully nobody does > > so, but it's possible), and because presubmits trigger much later in the > > development process than compile time assertions. Given that this is reasonably > > simple and self contained in one anonymous namespace block, I think this > > approach strikes the best overall balance between the various options. What do > > you think? > > I would personally prefer a presubmit, as this code is confusing and seems fragile if any part of PageLoadTiming internals changes (or the time classes change). > > However, I don't feel strongly enough to block this, so lgtm if you prefer this way. Yeah, it's a good point - I will take a look at the presubmit and see what's possible there. Will follow up soon. |
