Replace for loops with |arraysize| with for each loops

Replace for loops with |arraysize| with for each loops

This change replaces for-loops involving a loop variable and the
|arraysize| macro with a C++11 for-each-loop where appropriate.

A few usages of |arraysize| in a for-loop remain, in these the loop
variable is used as well. This can be simply logging the iteration
number or indexing another array of the same size.

BUG=655950
Committed: https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642
Cr-Commit-Position: refs/heads/master@{#425651}

[jdoerrie, 2016-10-14 11:16:16]

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

[jdoerrie, 2016-10-14 11:16:40]

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

[commit-bot: I haz the power, 2016-10-14 11:16:46]

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

[jdoerrie, 2016-10-14 11:25:26]

The remaining usages of |arraysize| in a loop are the following:

content/renderer/password_form_conversion_utils_browsertest.cc
326:  for (size_t i = 0; i < arraysize(cases); ++i) {
401:  for (size_t i = 0; i < arraysize(cases); ++i) {
461:  for (size_t i = 0; i < arraysize(cases); ++i) {
815:  for (size_t i = 0; i < arraysize(cases); ++i) {
1020:  for (size_t i = 0; i < arraysize(cases); ++i) {
1353:  for (size_t i = 0; i < arraysize(test_cases); ++i) {

core/browser/autofill_manager_unittest.cc
4210:  for (size_t i = 0; i < arraysize(test_fields); ++i) {

core/browser/contact_info_unittest.cc
228:  for (size_t i = 0; i < arraysize(test_cases); ++i) {
315:  for (size_t i = 0; i < arraysize(test_cases); ++i) {
367:  for (size_t i = 0; i < arraysize(test_cases); ++i) {

core/browser/autofill_profile_unittest.cc
376:  for (size_t i = 0; i < arraysize(kExpectedLabels); ++i) {
424:  for (size_t i = 0; i < arraysize(kExpectedLabels); ++i) {
477:  for (size_t i = 0; i < arraysize(kExpectedLabels); ++i) {
523:  for (size_t i = 0; i < arraysize(kExpectedLabels); ++i) {
565:  for (size_t i = 0; i < arraysize(kExpectedLabels); ++i) {

core/browser/webdata/autofill_table_unittest.cc
244:  for (size_t i = 0; i < arraysize(kExpectedChanges); ++i) {

core/browser/phone_field.cc
142:  for (size_t i = 0; i < arraysize(kPhoneFieldGrammars); ++i) {
147:    for (; i < arraysize(kPhoneFieldGrammars) &&
162:    if (i >= arraysize(kPhoneFieldGrammars)) {
172:    } while (i < arraysize(kPhoneFieldGrammars) &&
176:    if (i + 1 == arraysize(kPhoneFieldGrammars)) {

In all of these cases the loop variable is either logged or used to index
another array. In principle one could also use range based loops here (e.g. see
http://stackoverflow.com/a/11328545), but if you start to do this, almost every
other for loop should be changed as well for consistency reasons.

[commit-bot: I haz the power, 2016-10-14 12:07:26]

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

[commit-bot: I haz the power, 2016-10-14 12:07:26]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-10-17 07:40:33]

LGTM with comments.

Thanks for the analysis!
I agree that we should keep an explicit counter variable and arraysize in loops
where the index is needed, using std::distance looks like overkill.

Cheers,
Vaclav

https://codereview.chromium.org/2417783004/diff/1/components/autofill/content...
File components/autofill/content/renderer/form_cache.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/content...
components/autofill/content/renderer/form_cache.cc:48: for (const auto& str :
deprecated) {
When |str| is a pointer, we prefer to spell it out (* instead of &), see
http://crbug.com/554600. So a first step would be to have const auto*. Also,
while the pointer is the same size as a reference to copy, but having the
pointer reduces one step of indirection compared to the reference (looking up
what is being referenced) inside the loop body.

Second, the type |char| is simple enough to have no need for auto. Please just
use "const char* str: deprecated", it allows better understanding of the type
from that line without increasing code complexity.

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/address_unittest.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/address_unittest.cc:135: for (const auto&
valid_match : kValidMatches) {
const char* valid_match...

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/address_unittest.cc:147: for (const auto&
invalid_match : kInvalidMatches) {
const char*

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_profile.cc:357: for (const auto& type
: types) {
const auto& -> ServerFieldType

(1) Replacing auto with the (simple-named) type to make it clearer.

(2) Making a const ref to a value copy for an integer-like type, because it
removes one layer of indirection while preserving copying costs.

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/credit_card.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/credit_card.cc:577: for (const auto& type :
types) {
const auto& -> ServerFieldType

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/credit_card_unittest.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/credit_card_unittest.cc:536: for (const auto&
valid_number : kValidNumbers) {
const char*

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/credit_card_unittest.cc:541: for (const auto&
invalid_number : kInvalidNumbers) {
const char*

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/phone_field_unittest.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/phone_field_unittest.cc:82: for (const auto&
field_type : kFieldTypes) {
const char*

(Also everywhere below.)

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/validation_unittest.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/validation_unittest.cc:95: for (const auto&
valid_number : kValidNumbers) {
const char*

(Also below in this file.)

[jdoerrie, 2016-10-17 08:42:27]

https://codereview.chromium.org/2417783004/diff/1/components/autofill/content...
File components/autofill/content/renderer/form_cache.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/content...
components/autofill/content/renderer/form_cache.cc:48: for (const auto& str :
deprecated) {
On 2016/10/17 07:40:33, vabr (Chromium) wrote:
> When |str| is a pointer, we prefer to spell it out (* instead of &), see
> http://crbug.com/554600. So a first step would be to have const auto*. Also,
> while the pointer is the same size as a reference to copy, but having the
> pointer reduces one step of indirection compared to the reference (looking up
> what is being referenced) inside the loop body.
> 
> Second, the type |char| is simple enough to have no need for auto. Please just
> use "const char* str: deprecated", it allows better understanding of the type
> from that line without increasing code complexity.

Done.

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/address_unittest.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/address_unittest.cc:135: for (const auto&
valid_match : kValidMatches) {
On 2016/10/17 07:40:33, vabr (Chromium) wrote:
> const char* valid_match...

Done.

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/address_unittest.cc:147: for (const auto&
invalid_match : kInvalidMatches) {
On 2016/10/17 07:40:33, vabr (Chromium) wrote:
> const char*

Done.

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_profile.cc:357: for (const auto& type
: types) {
On 2016/10/17 07:40:33, vabr (Chromium) wrote:
> const auto& -> ServerFieldType
> 
> (1) Replacing auto with the (simple-named) type to make it clearer.
> 
> (2) Making a const ref to a value copy for an integer-like type, because it
> removes one layer of indirection while preserving copying costs.

Done.

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/credit_card.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/credit_card.cc:577: for (const auto& type :
types) {
On 2016/10/17 07:40:33, vabr (Chromium) wrote:
> const auto& -> ServerFieldType

Done.

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/credit_card_unittest.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/credit_card_unittest.cc:536: for (const auto&
valid_number : kValidNumbers) {
On 2016/10/17 07:40:33, vabr (Chromium) wrote:
> const char*

Done.

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/credit_card_unittest.cc:541: for (const auto&
invalid_number : kInvalidNumbers) {
On 2016/10/17 07:40:33, vabr (Chromium) wrote:
> const char*

Done.

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/phone_field_unittest.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/phone_field_unittest.cc:82: for (const auto&
field_type : kFieldTypes) {
On 2016/10/17 07:40:33, vabr (Chromium) wrote:
> const char*
> 
> (Also everywhere below.)

Done.

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/validation_unittest.cc (right):

https://codereview.chromium.org/2417783004/diff/1/components/autofill/core/br...
components/autofill/core/browser/validation_unittest.cc:95: for (const auto&
valid_number : kValidNumbers) {
On 2016/10/17 07:40:33, vabr (Chromium) wrote:
> const char*
> 
> (Also below in this file.)

Done.

[jdoerrie, 2016-10-17 09:15:52]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2016-10-17 09:15:53]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2417783004/#ps20001
(title: "Adressed Comments")

[commit-bot: I haz the power, 2016-10-17 09:16:16]

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-10-17 10:46:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-17 10:48:14]

Description was changed from

==========
Replace for loops with |arraysize| with for each loops

This change replaces for-loops involving a loop variable and the
|arraysize| macro with a C++11 for-each-loop where appropriate.

A few usages of |arraysize| in a for-loop remain, in these the loop
variable is used as well. This can be simply logging the iteration
number or indexing another array of the same size.

BUG=655950
==========

to

==========
Replace for loops with |arraysize| with for each loops

This change replaces for-loops involving a loop variable and the
|arraysize| macro with a C++11 for-each-loop where appropriate.

A few usages of |arraysize| in a for-loop remain, in these the loop
variable is used as well. This can be simply logging the iteration
number or indexing another array of the same size.

BUG=655950

Committed: https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642
Cr-Commit-Position: refs/heads/master@{#425651}
==========

[commit-bot: I haz the power, 2016-10-17 10:48:16]

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

[henrika (OOO until Aug 14), 2016-10-17 12:15:08]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2424793002/ by henrika@chromium.org.

The reason for reverting is: Speculative revert from sheriff. Suspect that the
CL breaks components_unittests on Mac.

https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests%...

Not sure. Will revert the revert if it does not help..

[henrika (OOO until Aug 14), 2016-10-17 12:31:20]

My revert landed at 425651 and 425657 is green.
Looks like my revert was OK in this case.