Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
382243002:
Cleanup return values in autofill (Closed)
|
Created:
6 years, 5 months ago by lucinka.brozkova Modified:
6 years, 5 months ago CC:
chromium-reviews, benquan, jam, browser-components-watch_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionCleanup return values in autofill
This CL turns const by value return values into const by ref.
Replacing const value by a const ref when a member variable is safe, because
the member variable will live at least as long as the temporary object returned
now. It can also be more efficient, because returning a ref can spare copying.
BUG=393155
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282769
Patch Set 1 #
Total comments: 14
Patch Set 2 : Revert unwanted changes #
Messages
Total messages: 9 (0 generated)
The compiler errors made me re-think my initial off-line review :), please correct. The AutofillCountry accessors seem fine to return by const ref (each pointed object's lifetime very likely extends that of the corresponding temporary object returned by those accessors now). Also, please correct the CL description: "Save change" should have been "Safe change", and probably deserves more explanation on the lines that: (1) Replacing const value by a const ref when a member variable is safe, because the member variable will live at least as long as the temporary object returned now. (2) Dropping the const when returning by value does not change anything -- the caller gets a new copy through which nothing can be changed in the called object. Keeping the return value non-const looks more flexible and less confusing. Once you address the comments, please add an OWNER for review, e.g., isherman@. Cheers, Vaclav https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:678: const base::string16& GetFormIdentifier(const WebFormElement& form) { Here returning a reference won't work, because the object returned lives on the local stack and goes out of scope after returning. Please return a (non-const) string16 by value instead. https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.h:80: const base::string16& GetFormIdentifier(const blink::WebFormElement& form); Could this method be const? https://codereview.chromium.org/382243002/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_profile.cc:484: const base::string16& AutofillProfile::PrimaryValue() const { Here again, the returned object is local and goes out of scope after the return. Please return by non-const value instead. https://codereview.chromium.org/382243002/diff/1/components/autofill/core/bro... File components/autofill/core/browser/credit_card.cc (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/core/bro... components/autofill/core/browser/credit_card.cc:431: return label; And here as well, the returned object is local, so please return by a non-const value.
https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:678: const base::string16& GetFormIdentifier(const WebFormElement& form) { On 2014/07/11 13:37:43, vabr (Chromium) wrote: > Here returning a reference won't work, because the object returned lives on the > local stack and goes out of scope after returning. > Please return a (non-const) string16 by value instead. As I mentioned on the bug, the const here is correct. It prevents a class of improbable but very surprising code snippets from compiling. https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.h:80: const base::string16& GetFormIdentifier(const blink::WebFormElement& form); On 2014/07/11 13:37:43, vabr (Chromium) wrote: > Could this method be const? This is a free function, not a method.
https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:678: const base::string16& GetFormIdentifier(const WebFormElement& form) { On 2014/07/11 19:11:10, Ilya Sherman wrote: > On 2014/07/11 13:37:43, vabr (Chromium) wrote: > > Here returning a reference won't work, because the object returned lives on > the > > local stack and goes out of scope after returning. > > Please return a (non-const) string16 by value instead. > > As I mentioned on the bug, the const here is correct. It prevents a class of > improbable but very surprising code snippets from compiling. Thanks for pointing that out. Let's revert the changes to form_autofill_util.*, then. https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.h:80: const base::string16& GetFormIdentifier(const blink::WebFormElement& form); On 2014/07/11 19:11:10, Ilya Sherman wrote: > On 2014/07/11 13:37:43, vabr (Chromium) wrote: > > Could this method be const? > > This is a free function, not a method. Ah thanks, my bad for missing that. I take my earlier comment back, since it does not make any sense.
https://codereview.chromium.org/382243002/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_profile.cc:484: const base::string16& AutofillProfile::PrimaryValue() const { On 2014/07/11 13:37:43, vabr (Chromium) wrote: > Here again, the returned object is local and goes out of scope after the return. > Please return by non-const value instead. And retracting my comment here as well, please revert the change instead, for the same reason as ToString(). https://codereview.chromium.org/382243002/diff/1/components/autofill/core/bro... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/core/bro... components/autofill/core/browser/credit_card.h:63: const base::string16& Label() const; So I guess the argument with ToString() applies here as well, let's revert changes to this file (and its .cc) as well.
Hi Ilya, Thank you for your review. I reverted all the files apart from autofill_country.h. Please have another look. Can you please also run some try bots on the patch? I don't have the rights to run them yet. Cheers, Lucie https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:678: const base::string16& GetFormIdentifier(const WebFormElement& form) { On 2014/07/11 19:24:47, vabr (Chromium) wrote: > On 2014/07/11 19:11:10, Ilya Sherman wrote: > > On 2014/07/11 13:37:43, vabr (Chromium) wrote: > > > Here returning a reference won't work, because the object returned lives on > > the > > > local stack and goes out of scope after returning. > > > Please return a (non-const) string16 by value instead. > > > > As I mentioned on the bug, the const here is correct. It prevents a class of > > improbable but very surprising code snippets from compiling. > > Thanks for pointing that out. > Let's revert the changes to form_autofill_util.*, then. Done. https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.h:80: const base::string16& GetFormIdentifier(const blink::WebFormElement& form); On 2014/07/11 19:24:47, vabr (Chromium) wrote: > On 2014/07/11 19:11:10, Ilya Sherman wrote: > > On 2014/07/11 13:37:43, vabr (Chromium) wrote: > > > Could this method be const? > > > > This is a free function, not a method. > > Ah thanks, my bad for missing that. > I take my earlier comment back, since it does not make any sense. Acknowledged. https://codereview.chromium.org/382243002/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_profile.cc:484: const base::string16& AutofillProfile::PrimaryValue() const { On 2014/07/11 19:27:49, vabr (Chromium) wrote: > On 2014/07/11 13:37:43, vabr (Chromium) wrote: > > Here again, the returned object is local and goes out of scope after the > return. > > Please return by non-const value instead. > > And retracting my comment here as well, please revert the change instead, for > the same reason as ToString(). Done. https://codereview.chromium.org/382243002/diff/1/components/autofill/core/bro... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/382243002/diff/1/components/autofill/core/bro... components/autofill/core/browser/credit_card.h:63: const base::string16& Label() const; On 2014/07/11 19:27:49, vabr (Chromium) wrote: > So I guess the argument with ToString() applies here as well, let's revert > changes to this file (and its .cc) as well. Done.
LGTM. Thanks for the cleanup!
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lucinka.brozkova@gmail.com/382243002/2...
Message was sent while issue was closed.
Change committed as 282769 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
