components: Change auto to not deduce raw pointers.

components: Change auto to not deduce raw pointers.

This patch updates the code to prevent auto deducing to a raw pointer.

R=jochen, danakj, dcheng
BUG=554600
Committed: https://crrev.com/2de366b533c783f31c6233f4034a8fa070b29db9
Cr-Commit-Position: refs/heads/master@{#406674}

[vmpstr, 2016-06-28 21:28:12]

Please take a look.

[vabr (Chromium), 2016-06-29 07:45:52]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2016-06-29 07:45:53]

I'm worried about auto& -> auto* in the case of ScopedVector, please correct me
if I'm wrong:

Before:

ScopedVector<T> v;
for (auto& p : v) {
  delete p;
  p = nullptr
}

After:

ScopedVector<T> v;
for (auto* p : v) {
  delete p;
  p = nullptr
}

It seems to me that in the "After" case, after the for-loop, |v| contains
dangling pointers. Is that so? There are a couple of instances of this in the
CL.

https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
File
components/password_manager/core/browser/credential_manager_pending_request_task.cc
(right):

https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:100:
form = nullptr;
This used to null the form in |results|. Now this only nulls the copy of that
pointer, with |results| staying untouched. Does that create a use-after-free
possibility?

https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
File components/password_manager/core/browser/password_manager_util.cc (right):

https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_util.cc:74: form =
nullptr;
ditto

https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
components/password_manager/core/browser/password_store.cc:57: login = nullptr;
ditto

[vabr (Chromium), 2016-06-29 07:50:56]

On 2016/06/29 07:45:53, vabr (Chromium) wrote:
> I'm worried about auto& -> auto* in the case of ScopedVector, please correct
me
> if I'm wrong:
> 
> Before:
> 
> ScopedVector<T> v;
> for (auto& p : v) {
>   delete p;
>   p = nullptr
> }
> 
> After:
> 
> ScopedVector<T> v;
> for (auto* p : v) {
>   delete p;
>   p = nullptr
> }
> 
> It seems to me that in the "After" case, after the for-loop, |v| contains
> dangling pointers. Is that so? There are a couple of instances of this in the
> CL.
> 
>
https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
> File
>
components/password_manager/core/browser/credential_manager_pending_request_task.cc
> (right):
> 
>
https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
>
components/password_manager/core/browser/credential_manager_pending_request_task.cc:100:
> form = nullptr;
> This used to null the form in |results|. Now this only nulls the copy of that
> pointer, with |results| staying untouched. Does that create a use-after-free
> possibility?
> 
>
https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
> File components/password_manager/core/browser/password_manager_util.cc
(right):
> 
>
https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
> components/password_manager/core/browser/password_manager_util.cc:74: form =
> nullptr;
> ditto
> 
>
https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
> File components/password_manager/core/browser/password_store.cc (right):
> 
>
https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
> components/password_manager/core/browser/password_store.cc:57: login =
nullptr;
> ditto

Not LGTM until the above concern gets clarified.

[noyau (Ping after 24h), 2016-06-29 08:55:33]

On 2016/06/28 21:28:12, vmpstr wrote:
> Please take a look.

Is there a more detailed documentation of why this sweeping change should be
made? The description is quite succinct, and it would help to have an example of
the problem you are trying to solve. Looking at the CL I'm failing to see why
you removed some of the perfectly fine const auto&. I agree that some
unqualified auto should be revised, but most likely by a by a const auto& to
avoid a copy, not a auto*.

I'm failing to see why going all out on auto* is a progress in readability
and/or correctness. What am I missing?

[vmpstr, 2016-06-29 18:38:26]

On 2016/06/29 07:45:53, vabr (Chromium) wrote:
> I'm worried about auto& -> auto* in the case of ScopedVector, please correct
me
> if I'm wrong:
> 
> Before:
> 
> ScopedVector<T> v;
> for (auto& p : v) {
>   delete p;
>   p = nullptr
> }
> 
> After:
> 
> ScopedVector<T> v;
> for (auto* p : v) {
>   delete p;
>   p = nullptr
> }
> 
> It seems to me that in the "After" case, after the for-loop, |v| contains
> dangling pointers. Is that so? There are a couple of instances of this in the
> CL.
> 

You're absolutely right. This patch was a result of the clang plugin
recommendations, but I think there's a pretty big flaw in it that it strips
(all) references. It should certainly keep non-const references when suggesting
replacements, which would then end up looking like auto*& which is at least a
signal to the author/reviewer to take a look at what's going on. I'll work on
updating the clang plugin and then get back to this when it's fixed. 

>
https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
> File
>
components/password_manager/core/browser/credential_manager_pending_request_task.cc
> (right):
> 
>
https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
>
components/password_manager/core/browser/credential_manager_pending_request_task.cc:100:
> form = nullptr;
> This used to null the form in |results|. Now this only nulls the copy of that
> pointer, with |results| staying untouched. Does that create a use-after-free
> possibility?
> 
>
https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
> File components/password_manager/core/browser/password_manager_util.cc
(right):
> 
>
https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
> components/password_manager/core/browser/password_manager_util.cc:74: form =
> nullptr;
> ditto
> 
>
https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
> File components/password_manager/core/browser/password_store.cc (right):
> 
>
https://codereview.chromium.org/2110663002/diff/1/components/password_manager...
> components/password_manager/core/browser/password_store.cc:57: login =
nullptr;
> ditto

[vmpstr, 2016-06-29 18:42:30]

On 2016/06/29 08:55:33, noyau wrote:
> On 2016/06/28 21:28:12, vmpstr wrote:
> > Please take a look.
> 
> Is there a more detailed documentation of why this sweeping change should be
> made? The description is quite succinct, and it would help to have an example
of
> the problem you are trying to solve. Looking at the CL I'm failing to see why
> you removed some of the perfectly fine const auto&. I agree that some
> unqualified auto should be revised, but most likely by a by a const auto& to
> avoid a copy, not a auto*.
> 
> I'm failing to see why going all out on auto* is a progress in readability
> and/or correctness. What am I missing?

My main motivation for this is to amend the style guide, letting developers use
auto for smart pointers. Specifically, the current situation is that

auto foo = CreateFoo();

makes it unclear whether |foo| is a raw pointer or a smart pointer, making it
difficult to figure out what is the proper way to clean this up.

With this change, and the subsequent change to enable the no-auto-raw-pointer
check by default, the line above would only compile if CreateFoo() returns a
smart pointer. With a raw pointer, you'd get an error along the lines of:

warning: [chromium-style] auto variable type must not deduce to a raw pointer
type. 
auto foo = CreateFoo();
^~~~
auto*

[vabr (Chromium), 2016-06-30 07:53:05]

Hi vmpstr!

Thanks for the response about my concerns (and also for the explanation in reply
to noyau@). What you write makes good sense to me. I'm fine with the shift from
auto& to auto*& for pointer references and see this change as improvement.

Please do let me know once your CLs need a re-review, and I'll be happy to have
another look.

Cheers,
Vaclav

[vmpstr, 2016-07-18 22:17:05]

Hey, I think this is all fixed up. Sorry it took a while. Please take another
look! Thanks.

[vabr (Chromium), 2016-07-19 07:54:09]

Thanks for the clean-up!

autofill and password_manager components LGTM (let me know if you want me to
review other code, but I'm only OWNER of those two.

I left some comments, but you are free to ignore them, because they are aimed at
separate issues than you target, and the issues have been in the code before
your CL:
 * Some nits suggesting auto* -> const auto* where the derived auto is already a
const type.
 * Some optional suggestions where the semantics of the code would be changed,
but seems like a nice and easy clean-up.
 * Some optional nits fixing naming issues.

Thanks!
Vaclav

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1148: for (auto* iter
: form_fields)
optional: This could even be "const auto*".

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1553: for (auto*
keyword : kKeywords) {
nit: "const auto*" to emphasise that |keyword| points in fact to a const object?

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1060: for (auto* field :
submitted_form) {
optional: Make this "const auto*" to point out that the pointee is not modified
in the for loop?

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:256: for (auto* it : info)
{
nit: "const auto*" to emphasise the actual type of |it|?

Also below on line 701.

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:256: for (auto* it : info)
{
optional nit: Since |it| is not an iterator, could we rename it to |group| or
|form_group|?

Also below on line 701.

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:435: for (auto* form : forms)
{
optional: Could we make this "const auto*" to clarify that the pointee is not
changed inside the loop?

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:594: for (auto* it :
server_credit_cards_) {
optional: Could we make this "const auto*" and change the type of
|existing_credit_card| to "const CreditCard*" to point out that the pointee is
not changed?

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:594: for (auto* it :
server_credit_cards_) {
optional nit: Could we rename |it| to |server_card| or something similar not
containing hints at iterators?

Also below on line 623.

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
(right):

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:114:
for (auto* p : profiles_) {
optional: "const auto*"

https://codereview.chromium.org/2110663002/diff/20001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_request_task.cc
(right):

https://codereview.chromium.org/2110663002/diff/20001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:30:
for (auto* form : local_results)
optional: "const auto*"

https://codereview.chromium.org/2110663002/diff/20001/components/password_man...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/2110663002/diff/20001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:847: for
(auto* form : result)
optional: "const auto*"

Also below on line 868.

[jochen (gone - plz use gerrit), 2016-07-19 12:28:22]

danakj, does the patch look good? Happy to rubberstamp if it does

[vabr (Chromium), 2016-07-19 13:55:18]

https://codereview.chromium.org/2110663002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2110663002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager.cc:502: for (auto*
old_manager : old_login_managers) {
Heads-up: https://codereview.chromium.org/2157333002/ just landed and the
for-loop now runs over vector<unique_ptr> instead of ScopedVector. That means
that auto* won't compile in the rebased code, because the element of the
collection is not a pointer.

It is safe to just drop your changes to this file during rebasing.

[Primiano Tucci (use gerrit), 2016-07-19 14:05:47]

Is there any plan to have a clang plugin for this? if not can we have at least a
note in chromium-cpp.appspot.com?
The Google C++ style guide doesn't mention this. I feel this will regress again.
I actually wrote (and reviewed) code like "for (auto x : vector_of_pointers)"
thinking it was fine, TIL it's not just because I was in a watchlist for this
CL.

[vmpstr, 2016-07-19 18:17:30]

On 2016/07/19 14:05:47, Primiano Tucci wrote:
> Is there any plan to have a clang plugin for this? if not can we have at least
a
> note in chromium-cpp.appspot.com?
> The Google C++ style guide doesn't mention this. I feel this will regress
again.
> I actually wrote (and reviewed) code like "for (auto x : vector_of_pointers)"
> thinking it was fine, TIL it's not just because I was in a watchlist for this
> CL.

These changes are a result of a clang plugin. I'm fixing existing code issues
after which I'm hoping to enable the plugin by default. If you're interested,
the check-auto-raw-pointer flag for the find bad constructs plugin is the thing
doing the work.

[danakj, 2016-07-19 18:49:25]

On 2016/07/19 12:28:22, jochen wrote:
> danakj, does the patch look good? Happy to rubberstamp if it does

Yes LGTM.

I agree, it'd be nice if all const auto& became const auto* where they compile.

[vmpstr, 2016-07-19 19:43:24]

The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 19:44:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2016-07-19 19:49:31]

PTAL. The reason I left it as auto* instead of const auto* is that const auto&
would make the object pointed to by the pointer mutable, and I didn't want to
change semantics. However, I've added const throughout the patch when it seemed
appropriate. Thanks

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1148: for (auto* iter
: form_fields)
On 2016/07/19 07:54:08, vabr (Chromium) wrote:
> optional: This could even be "const auto*".

Done.

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1553: for (auto*
keyword : kKeywords) {
On 2016/07/19 07:54:08, vabr (Chromium) wrote:
> nit: "const auto*" to emphasise that |keyword| points in fact to a const
object?

Done.

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1060: for (auto* field :
submitted_form) {
On 2016/07/19 07:54:08, vabr (Chromium) wrote:
> optional: Make this "const auto*" to point out that the pointee is not
modified
> in the for loop?

Done.

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:256: for (auto* it : info)
{
On 2016/07/19 07:54:08, vabr (Chromium) wrote:
> optional nit: Since |it| is not an iterator, could we rename it to |group| or
> |form_group|?
> 
> Also below on line 701.

Done.

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:435: for (auto* form : forms)
{
On 2016/07/19 07:54:09, vabr (Chromium) wrote:
> optional: Could we make this "const auto*" to clarify that the pointee is not
> changed inside the loop?

Done.

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:594: for (auto* it :
server_credit_cards_) {
On 2016/07/19 07:54:09, vabr (Chromium) wrote:
> optional: Could we make this "const auto*" and change the type of
> |existing_credit_card| to "const CreditCard*" to point out that the pointee is
> not changed?

Done.

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
(right):

https://codereview.chromium.org/2110663002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:114:
for (auto* p : profiles_) {
On 2016/07/19 07:54:09, vabr (Chromium) wrote:
> optional: "const auto*"

Done.

https://codereview.chromium.org/2110663002/diff/20001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_request_task.cc
(right):

https://codereview.chromium.org/2110663002/diff/20001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:30:
for (auto* form : local_results)
On 2016/07/19 07:54:09, vabr (Chromium) wrote:
> optional: "const auto*"

Done.

https://codereview.chromium.org/2110663002/diff/20001/components/password_man...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/2110663002/diff/20001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:847: for
(auto* form : result)
On 2016/07/19 07:54:09, vabr (Chromium) wrote:
> optional: "const auto*"
> 
> Also below on line 868.

Done.

https://codereview.chromium.org/2110663002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2110663002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager.cc:502: for (auto*
old_manager : old_login_managers) {
On 2016/07/19 13:55:18, vabr (Chromium) wrote:
> Heads-up: https://codereview.chromium.org/2157333002/ just landed and the
> for-loop now runs over vector<unique_ptr> instead of ScopedVector. That means
> that auto* won't compile in the rebased code, because the element of the
> collection is not a pointer.
> 
> It is safe to just drop your changes to this file during rebasing.

Acknowledged.

[commit-bot: I haz the power, 2016-07-19 20:56:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 20:56:31]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-07-20 08:05:56]

LGTM, thank you!
Vaclav

[jochen (gone - plz use gerrit), 2016-07-20 11:09:06]

lgtm

[vmpstr, 2016-07-20 20:41:20]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2016-07-20 20:41:20]

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/2110663002/#ps80001
(title: "rebase+one fix")

[commit-bot: I haz the power, 2016-07-20 20:41:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 21:36:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-20 21:38:10]

Description was changed from

==========
components: Change auto to not deduce raw pointers.

This patch updates the code to prevent auto deducing to a raw pointer.

R=jochen, danakj, dcheng
BUG=554600
==========

to

==========
components: Change auto to not deduce raw pointers.

This patch updates the code to prevent auto deducing to a raw pointer.

R=jochen, danakj, dcheng
BUG=554600

Committed: https://crrev.com/2de366b533c783f31c6233f4034a8fa070b29db9
Cr-Commit-Position: refs/heads/master@{#406674}
==========

[commit-bot: I haz the power, 2016-07-20 21:38:13]

Patchset 5 (id:??) landed as
https://crrev.com/2de366b533c783f31c6233f4034a8fa070b29db9
Cr-Commit-Position: refs/heads/master@{#406674}