|
|
Created:
4 years ago by vabr (Chromium) Modified:
3 years, 10 months ago CC:
chromium-reviews, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInline FundamentalValue into base::Value
This is forked from patch from issue 2475603002 at patchset 20001
(http://crrev.com/2475603002#ps20001). The first patchset here is the original
+ git cl format.
BUG=646113
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Copy and move constructor added #Patch Set 4 : tests #Patch Set 5 : Comment fixed #
Total comments: 4
Patch Set 6 : EXPECT_EQ(boolean literal...) -> EXPECT_{TRUE|FALSE}(...) #Patch Set 7 : explicit #
Total comments: 4
Patch Set 8 : Remove {} from if #
Total comments: 2
Patch Set 9 : Just rebased #Patch Set 10 : Value(Type) initializes with the default value #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 51 (39 generated)
The CQ bit was checked by vabr@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vabr@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vabr@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 vabr@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 vabr@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 vabr@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...
vabr@chromium.org changed reviewers: + brettw@chromium.org, jdoerrie@chromium.org
Brett, Jan, Feel free to leave comments here. Cheers, Vaclav
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by vabr@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...
Left a few comments. thanks Vaclav! https://codereview.chromium.org/2558023002/diff/80001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2558023002/diff/80001/base/values.cc#newcode275 base/values.cc:275: InternalCopyFrom(that); Just out of curiosity: How should we handle Values that are moved out of? Is setting the type to NONE a good idea, especially when it's not a FundamentalValue? https://codereview.chromium.org/2558023002/diff/80001/base/values.h File base/values.h (right): https://codereview.chromium.org/2558023002/diff/80001/base/values.h#newcode142 base/values.h:142: Value(double in_double); These constructors probably should be explicit.
Thanks, Jan! Please rebase your patch on the newest patch set here, otherwise Android compilation will fail for you. Cheers, Vaclav https://codereview.chromium.org/2558023002/diff/80001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2558023002/diff/80001/base/values.cc#newcode275 base/values.cc:275: InternalCopyFrom(that); On 2016/12/08 10:13:08, jdoerrie wrote: > Just out of curiosity: How should we handle Values that are moved out of? Is > setting the type to NONE a good idea, especially when it's not a > FundamentalValue? There is no obligation to do anything to the storage from which me moved the value away. We can also do anything to it as long as the resulting storage will be able to be deleted or assigned to. In general we should do nothing, just to spare processing effort. Having said that, setting type to NONE should work and could help us find some bugs for a reasonably small effort. If we change the type, getting the deletion right might need some care for StringValue and beyond, where we actually need to call Destroy on the ManualConstructor, based on the type. So I propose to not introduce it with FundamentalValue, but see if it would work once StringValue is inlined. From there one we can just carry on with whatever we decided to do for the rest of the values. https://codereview.chromium.org/2558023002/diff/80001/base/values.h File base/values.h (right): https://codereview.chromium.org/2558023002/diff/80001/base/values.h#newcode142 base/values.h:142: Value(double in_double); On 2016/12/08 10:13:08, jdoerrie wrote: > These constructors probably should be explicit. Done.
The CQ bit was checked by vabr@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm https://codereview.chromium.org/2558023002/diff/120001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2558023002/diff/120001/base/values.cc#newcode269 base/values.cc:269: InternalCopyFrom(that); Thanks for finding this was missing! Seems obvious in retrospect. https://codereview.chromium.org/2558023002/diff/120001/base/values.cc#newcode279 base/values.cc:279: if (this != &that) { For consistency with other code in this file, don't use {} for single-line conditionals.
Be sure to remove "[Copy of]" from the description before checkin.
Description was changed from ========== [Copy of] Inline FundamentalValue into base::Value This is forked from patch from issue 2475603002 at patchset 20001 (http://crrev.com/2475603002#ps20001). The first patchset here is the original + git cl format. The reason is that jdoerrie@ and vabr@ are working on patches depending on the above, and need to ensure that the base patch is updated frequently and passes tests, without the need to bother brettw@ with requests for updates. The plan is once the chain of dependent patches is ready, we can either update 2475603002 and land it, or land this one in its stead, whatever is preferred by brettw@. BUG=646113 ========== to ========== Inline FundamentalValue into base::Value This is forked from patch from issue 2475603002 at patchset 20001 (http://crrev.com/2475603002#ps20001). The first patchset here is the original + git cl format. BUG=646113 ==========
Thanks, Brett! Should I land this now, or would you prefer to have the cascade of CLs inlining all the Values ready before committing them? Cheers, Vaclav https://codereview.chromium.org/2558023002/diff/120001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2558023002/diff/120001/base/values.cc#newcode269 base/values.cc:269: InternalCopyFrom(that); On 2016/12/08 21:57:47, brettw (ping after 24h) wrote: > Thanks for finding this was missing! Seems obvious in retrospect. Chromium has apparently in some cases accidentally good tests! :) https://codereview.chromium.org/2558023002/diff/120001/base/values.cc#newcode279 base/values.cc:279: if (this != &that) { On 2016/12/08 21:57:47, brettw (ping after 24h) wrote: > For consistency with other code in this file, don't use {} for single-line > conditionals. Done.
The CQ bit was checked by vabr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2558023002/#ps140001 (title: "Remove {} from if")
The CQ bit was unchecked by vabr@chromium.org
The CQ bit was checked by vabr@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.
As I told Jan, we don't need to wait until everything is done before landing any CL (that would be difficult). But I was concerned about the risk of losing interest halfway through and the code being in worse shape overall. My suggestion was to figure out StringValue as a "we're really committing to finish" level of work, land everything so far, then finish the rest.
On 2016/12/09 20:42:24, brettw (ping after 24h) wrote: > As I told Jan, we don't need to wait until everything is done before landing any > CL (that would be difficult). But I was concerned about the risk of losing > interest halfway through and the code being in worse shape overall. My > suggestion was to figure out StringValue as a "we're really committing to > finish" level of work, land everything so far, then finish the rest. SGTM, will do that. Thanks!
I just reread the proposal and found another point worth discussing. https://codereview.chromium.org/2558023002/diff/140001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2558023002/diff/140001/base/values.cc#newcode266 base/values.cc:266: Value::Value(Type type) : type_(type) {} According to the proposal this constructor should default initialize the corresponding data member: """ explicit Value(Type type); // Does default value for this type. """ Right now it is protected, so customers would not be able to use it, but maybe it's worth implementing regardless? There is a chance a lot of currently protected constructors will become public when the inline StringValue CL lands.
The CQ bit was checked by vabr@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 vabr@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/2558023002/diff/140001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2558023002/diff/140001/base/values.cc#newcode266 base/values.cc:266: Value::Value(Type type) : type_(type) {} On 2016/12/23 14:18:26, jdoerrie wrote: > According to the proposal this constructor should default initialize the > corresponding data member: > > """ > explicit Value(Type type); // Does default value for this type. > """ > > Right now it is protected, so customers would not be able to use it, but maybe > it's worth implementing regardless? There is a chance a lot of currently > protected constructors will become public when the inline StringValue CL lands. Seems fine with me, done. brettw@ -- please let me know if you disagree (or have comments to the particular way I wrote this).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Vaclav! I left a comment on the usage of NOTREACHED. https://codereview.chromium.org/2558023002/diff/180001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2558023002/diff/180001/base/values.cc#newcode295 base/values.cc:295: return; Since the compiler already enforces that we cover all cases in the switch statement I think we can remove NOTREACHED() here. The other usages of NOTREACHED() in this file indicate logic errors; either we forgot to override a method of the base class or we called Value(double) with a non-finite argument. If you disagree and think there is merit in keeping it (e.g. for enforcing we don't forget to return from within the switch statement), you should also add NOTREACHED() at the bottom of Value::InternalCopyFrom for consistency.
I am closing this, because it was replaced by https://codereview.chromium.org/2645073002/. |