Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2486053002:
Switch to C++ 11 for-loop in l10n_util.cc (Closed)
|
Created:
4 years, 1 month ago by sanghee Modified:
4 years, 1 month ago Reviewers:
jungshik at Google CC:
chromium-reviews, jshin+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch to C++ 11 for-loop in l10n_util.cc
This change is to replace for-loop involving arraysize() with
C+11 for-loop.
file : ui/base/l10n/l10n_util.cc
BUG=655950
Committed: https://crrev.com/39b139c1ed407353fd64683bdf69d6762ed09175
Cr-Commit-Position: refs/heads/master@{#431333}
Patch Set 1 #
Total comments: 3
Patch Set 2 : This change is to replace for-loop involving arraysize with for-each-loop. #Messages
Total messages: 19 (7 generated)
Description was changed from ========== Replace for each loop in l10n_util.cc s change replaces for-loops involving a loop variable and the |arraysize| macro with a C++11 for-each-loop where appropriate. file : Replace for each loop in l10n_util.cc BUG=655950 ========== to ========== Replace for each loop in l10n_util.cc s change replaces for-loops involving a loop variable and the |arraysize| macro with a C++11 for-each-loop where appropriate. file : ui/base/l10n/l10n_util.cc BUG=655950 ==========
sanghee.lee1992@gmail.com changed reviewers: + jshin@chromium.org
Thank you
CL summary was a bit confusing. How about this? > Replace for each loop in l10n_util.cc Switch to C++ 11 for-loop in l10n_util.cc > s change replaces for-loops involving a loop variable and the > |arraysize| macro with a C++11 for-each-loop where appropriate. I guess you meant to say 'This change replaces ....'. :-) https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc File ui/base/l10n/l10n_util.cc (right): https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... ui/base/l10n/l10n_util.cc:218: for (const char* kduplicatename : kDuplicateNames) { nit: perhaps, |duplicate_name| would be better. https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... ui/base/l10n/l10n_util.cc:399: for (const auto& amap : alias_map) { nit: alias instead of amap ? https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... ui/base/l10n/l10n_util.cc:863: for (const char* kaccept_language : kAcceptLanguageList) { nit: |accept_language| instead of |kaccept_language| ?
On 2016/11/10 00:16:10, jungshik at google wrote: > CL summary was a bit confusing. How about this? > > > Replace for each loop in l10n_util.cc > > Switch to C++ 11 for-loop in l10n_util.cc > > > s change replaces for-loops involving a loop variable and the > > |arraysize| macro with a C++11 for-each-loop where appropriate. > > I guess you meant to say 'This change replaces ....'. :-) > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc > File ui/base/l10n/l10n_util.cc (right): > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... > ui/base/l10n/l10n_util.cc:218: for (const char* kduplicatename : > kDuplicateNames) { > nit: perhaps, |duplicate_name| would be better. > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... > ui/base/l10n/l10n_util.cc:399: for (const auto& amap : alias_map) { > nit: alias instead of amap ? > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... > ui/base/l10n/l10n_util.cc:863: for (const char* kaccept_language : > kAcceptLanguageList) { > nit: |accept_language| instead of |kaccept_language| ? Thank you for reviewing it
Description was changed from ========== Replace for each loop in l10n_util.cc s change replaces for-loops involving a loop variable and the |arraysize| macro with a C++11 for-each-loop where appropriate. file : ui/base/l10n/l10n_util.cc BUG=655950 ========== to ========== Replace for each loop in l10n_util.cc This change is to replace for-loop involving arraysize() with for-each-loop. file : ui/base/l10n/l10n_util.cc BUG=655950 ==========
On 2016/11/10 00:53:46, sanghee wrote: > On 2016/11/10 00:16:10, jungshik at google wrote: > > CL summary was a bit confusing. How about this? > > > > > Replace for each loop in l10n_util.cc > > > > Switch to C++ 11 for-loop in l10n_util.cc > > > > > s change replaces for-loops involving a loop variable and the > > > |arraysize| macro with a C++11 for-each-loop where appropriate. > > > > I guess you meant to say 'This change replaces ....'. :-) > > > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc > > File ui/base/l10n/l10n_util.cc (right): > > > > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... > > ui/base/l10n/l10n_util.cc:218: for (const char* kduplicatename : > > kDuplicateNames) { > > nit: perhaps, |duplicate_name| would be better. > > > > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... > > ui/base/l10n/l10n_util.cc:399: for (const auto& amap : alias_map) { > > nit: alias instead of amap ? > > > > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... > > ui/base/l10n/l10n_util.cc:863: for (const char* kaccept_language : > > kAcceptLanguageList) { > > nit: |accept_language| instead of |kaccept_language| ? > > Thank you for reviewing it Btw, you need to sign CLA. See https://cla.developers.google.com/about/google-individual?csw=1
One more: once CLA is signed, add yourself to AUTHORS file
On 2016/11/10 04:55:37, jungshik at google wrote: > On 2016/11/10 00:53:46, sanghee wrote: > > On 2016/11/10 00:16:10, jungshik at google wrote: > > > CL summary was a bit confusing. How about this? > > > > > > > Replace for each loop in l10n_util.cc > > > > > > Switch to C++ 11 for-loop in l10n_util.cc > > > > > > > s change replaces for-loops involving a loop variable and the > > > > |arraysize| macro with a C++11 for-each-loop where appropriate. > > > > > > I guess you meant to say 'This change replaces ....'. :-) > > > > > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc > > > File ui/base/l10n/l10n_util.cc (right): > > > > > > > > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... > > > ui/base/l10n/l10n_util.cc:218: for (const char* kduplicatename : > > > kDuplicateNames) { > > > nit: perhaps, |duplicate_name| would be better. > > > > > > > > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... > > > ui/base/l10n/l10n_util.cc:399: for (const auto& amap : alias_map) { > > > nit: alias instead of amap ? > > > > > > > > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... > > > ui/base/l10n/l10n_util.cc:863: for (const char* kaccept_language : > > > kAcceptLanguageList) { > > > nit: |accept_language| instead of |kaccept_language| ? > > > > Thank you for reviewing it > > > Btw, you need to sign CLA. See > https://cla.developers.google.com/about/google-individual?csw=1 I did before. Does not work it?
On 2016/11/10 05:06:12, sanghee wrote: > On 2016/11/10 04:55:37, jungshik at google wrote: > > On 2016/11/10 00:53:46, sanghee wrote: > > > On 2016/11/10 00:16:10, jungshik at google wrote: > > > > CL summary was a bit confusing. How about this? > > > > > > > > > Replace for each loop in l10n_util.cc > > > > > > > > Switch to C++ 11 for-loop in l10n_util.cc > > > > > > > > > s change replaces for-loops involving a loop variable and the > > > > > |arraysize| macro with a C++11 for-each-loop where appropriate. > > > > > > > > I guess you meant to say 'This change replaces ....'. :-) > > > > > > > > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc > > > > File ui/base/l10n/l10n_util.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... > > > > ui/base/l10n/l10n_util.cc:218: for (const char* kduplicatename : > > > > kDuplicateNames) { > > > > nit: perhaps, |duplicate_name| would be better. > > > > > > > > > > > > > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... > > > > ui/base/l10n/l10n_util.cc:399: for (const auto& amap : alias_map) { > > > > nit: alias instead of amap ? > > > > > > > > > > > > > > https://codereview.chromium.org/2486053002/diff/1/ui/base/l10n/l10n_util.cc#n... > > > > ui/base/l10n/l10n_util.cc:863: for (const char* kaccept_language : > > > > kAcceptLanguageList) { > > > > nit: |accept_language| instead of |kaccept_language| ? > > > > > > Thank you for reviewing it > > > > > > Btw, you need to sign CLA. See > > https://cla.developers.google.com/about/google-individual?csw=1 > > I signed CLA already
Description was changed from ========== Replace for each loop in l10n_util.cc This change is to replace for-loop involving arraysize() with for-each-loop. file : ui/base/l10n/l10n_util.cc BUG=655950 ========== to ========== Switch to C++ 11 for-loop in l10n_util.cc This change is to replace for-loop involving arraysize() with C+11 for-loop. file : ui/base/l10n/l10n_util.cc BUG=655950 ==========
LGTM
The CQ bit was checked by jshin@chromium.org
On 2016/11/10 17:58:59, jungshik at google wrote: > LGTM Thank you for the change. Adding to CQ
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Switch to C++ 11 for-loop in l10n_util.cc This change is to replace for-loop involving arraysize() with C+11 for-loop. file : ui/base/l10n/l10n_util.cc BUG=655950 ========== to ========== Switch to C++ 11 for-loop in l10n_util.cc This change is to replace for-loop involving arraysize() with C+11 for-loop. file : ui/base/l10n/l10n_util.cc BUG=655950 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Switch to C++ 11 for-loop in l10n_util.cc This change is to replace for-loop involving arraysize() with C+11 for-loop. file : ui/base/l10n/l10n_util.cc BUG=655950 ========== to ========== Switch to C++ 11 for-loop in l10n_util.cc This change is to replace for-loop involving arraysize() with C+11 for-loop. file : ui/base/l10n/l10n_util.cc BUG=655950 Committed: https://crrev.com/39b139c1ed407353fd64683bdf69d6762ed09175 Cr-Commit-Position: refs/heads/master@{#431333} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/39b139c1ed407353fd64683bdf69d6762ed09175 Cr-Commit-Position: refs/heads/master@{#431333} |
