|
|
Created:
3 years, 7 months ago by bcwhite Modified:
3 years, 7 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, danakj+watch_chromium.org, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCopy only accessed PersistentHistogramData fields when validating.
The DelayedPersistentAllocation added an atomic field to the
PersistentHistogramData structure that cannot be copied using
operator= (at least not without redefining it). Copies of only
some of the fields are needed so explicitly copy only those; the
atomic field is not one of them.
BUG=721352
Review-Url: https://codereview.chromium.org/2875643004
Cr-Commit-Position: refs/heads/master@{#471143}
Committed: https://chromium.googlesource.com/chromium/src/+/e0ecce46642e1d001deae09f41429c44517d84b9
Patch Set 1 #
Total comments: 4
Patch Set 2 : use local vars instead of local structure #Messages
Total messages: 20 (12 generated)
Description was changed from ========== Only copy accessed PersistentHistogramData fields when validating. The DelayedPersistentAllocation added an atomic field to the PersistentHistogramData structure that cannot be copied using operator= (at least not without redefining it). Copies of only some of the fields are needed so explicitly copy only those; the atomic field is not one of them. BUG=721352 ========== to ========== Copy only accessed PersistentHistogramData fields when validating. The DelayedPersistentAllocation added an atomic field to the PersistentHistogramData structure that cannot be copied using operator= (at least not without redefining it). Copies of only some of the fields are needed so explicitly copy only those; the atomic field is not one of them. BUG=721352 ==========
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
The CQ bit was checked by bcwhite@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...
https://codereview.chromium.org/2875643004/diff/1/base/metrics/persistent_his... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2875643004/diff/1/base/metrics/persistent_his... base/metrics/persistent_histogram_allocator.cc:575: PersistentHistogramData histogram_data; Can you add a CopyTo() function instead? This way, someone changing the struct can be sure to update it instead of the code living elsewhere (here).
https://codereview.chromium.org/2875643004/diff/1/base/metrics/persistent_his... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2875643004/diff/1/base/metrics/persistent_his... base/metrics/persistent_histogram_allocator.cc:575: PersistentHistogramData histogram_data; On 2017/05/11 20:20:07, Alexei Svitkine (slow) wrote: > Can you add a CopyTo() function instead? This way, someone changing the struct > can be sure to update it instead of the code living elsewhere (here). I looked at that path... but it was going to get complicated because the metadata structures should also use atomic copies and they're not needed for this code. Perhaps I should remove the local PHD instance and create individual variables instead?
https://codereview.chromium.org/2875643004/diff/1/base/metrics/persistent_his... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2875643004/diff/1/base/metrics/persistent_his... base/metrics/persistent_histogram_allocator.cc:575: PersistentHistogramData histogram_data; On 2017/05/11 20:23:16, bcwhite wrote: > On 2017/05/11 20:20:07, Alexei Svitkine (slow) wrote: > > Can you add a CopyTo() function instead? This way, someone changing the struct > > can be sure to update it instead of the code living elsewhere (here). > > I looked at that path... but it was going to get complicated because the > metadata structures should also use atomic copies and they're not needed for > this code. > > Perhaps I should remove the local PHD instance and create individual variables > instead? That SGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@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...
https://codereview.chromium.org/2875643004/diff/1/base/metrics/persistent_his... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2875643004/diff/1/base/metrics/persistent_his... base/metrics/persistent_histogram_allocator.cc:575: PersistentHistogramData histogram_data; On 2017/05/11 20:27:55, Alexei Svitkine (slow) wrote: > On 2017/05/11 20:23:16, bcwhite wrote: > > On 2017/05/11 20:20:07, Alexei Svitkine (slow) wrote: > > > Can you add a CopyTo() function instead? This way, someone changing the > struct > > > can be sure to update it instead of the code living elsewhere (here). > > > > I looked at that path... but it was going to get complicated because the > > metadata structures should also use atomic copies and they're not needed for > > this code. > > > > Perhaps I should remove the local PHD instance and create individual variables > > instead? > > That SGTM. Done.
lgtm
The CQ bit was unchecked by bcwhite@chromium.org
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494539351363440, "parent_rev": "459731e7f4376e99d785ac7b0d40622747ca318e", "commit_rev": "e0ecce46642e1d001deae09f41429c44517d84b9"}
Message was sent while issue was closed.
Description was changed from ========== Copy only accessed PersistentHistogramData fields when validating. The DelayedPersistentAllocation added an atomic field to the PersistentHistogramData structure that cannot be copied using operator= (at least not without redefining it). Copies of only some of the fields are needed so explicitly copy only those; the atomic field is not one of them. BUG=721352 ========== to ========== Copy only accessed PersistentHistogramData fields when validating. The DelayedPersistentAllocation added an atomic field to the PersistentHistogramData structure that cannot be copied using operator= (at least not without redefining it). Copies of only some of the fields are needed so explicitly copy only those; the atomic field is not one of them. BUG=721352 Review-Url: https://codereview.chromium.org/2875643004 Cr-Commit-Position: refs/heads/master@{#471143} Committed: https://chromium.googlesource.com/chromium/src/+/e0ecce46642e1d001deae09f4142... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e0ecce46642e1d001deae09f4142... |