Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2724153004:
Declare a variable before using it (Closed)
|
Created:
3 years, 9 months ago by chengx Modified:
3 years, 9 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, piman+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDeclare a variable before using it
PVS-Studio pointed out that an uninitialized variable 'limit' is used to initialize
itself, which may cause undefined behavior. Other calls to StringToSizeT() don't
have this issue.
BUG=697659
Review-Url: https://codereview.chromium.org/2724153004
Cr-Commit-Position: refs/heads/master@{#454604}
Committed: https://chromium.googlesource.com/chromium/src/+/15343248cec70d6da70eafc4cb87c1626d040349
Patch Set 1 #
Total comments: 3
Messages
Total messages: 33 (21 generated)
The CQ bit was checked by chengx@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...
Description was changed from ========== Format. Initialize the parameter before its usage. BUG=697659 ========== to ========== Declare a variable before using it PVS-Studio pointed out that an uninitialized variable 'limit' was used to initialize itself, which may cause undefined behavior. Other calls to StringToSizeT() don't have this issue. BUG=697659 ==========
Description was changed from ========== Declare a variable before using it PVS-Studio pointed out that an uninitialized variable 'limit' was used to initialize itself, which may cause undefined behavior. Other calls to StringToSizeT() don't have this issue. BUG=697659 ========== to ========== Declare a variable before using it PVS-Studio pointed out that an uninitialized variable 'limit' is used to initialize itself, which may cause undefined behavior. Other calls to StringToSizeT() don't have this issue. BUG=697659 ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by chengx@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 chengx@chromium.org
The CQ bit was checked by chengx@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
chengx@chromium.org changed reviewers: + jbauman@chromium.org
PTAL~
On 2017/03/03 00:12:03, chengx wrote: > PTAL~ I'm not an owner of this file.
The CQ bit was checked by chengx@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...
Patchset #1 (id:20001) has been deleted
chengx@chromium.org changed reviewers: + vabr@chromium.org - jbauman@chromium.org
PTAL~
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mathp@chromium.org changed reviewers: + mathp@chromium.org
lgtm
The CQ bit was checked by chengx@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488561018766180,
"parent_rev": "1f3d0d9a7f586ae7f53703d7c5593287b3847a31", "commit_rev":
"15343248cec70d6da70eafc4cb87c1626d040349"}
Message was sent while issue was closed.
Description was changed from ========== Declare a variable before using it PVS-Studio pointed out that an uninitialized variable 'limit' is used to initialize itself, which may cause undefined behavior. Other calls to StringToSizeT() don't have this issue. BUG=697659 ========== to ========== Declare a variable before using it PVS-Studio pointed out that an uninitialized variable 'limit' is used to initialize itself, which may cause undefined behavior. Other calls to StringToSizeT() don't have this issue. BUG=697659 Review-Url: https://codereview.chromium.org/2724153004 Cr-Commit-Position: refs/heads/master@{#454604} Committed: https://chromium.googlesource.com/chromium/src/+/15343248cec70d6da70eafc4cb87... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:40001) as https://chromium.googlesource.com/chromium/src/+/15343248cec70d6da70eafc4cb87...
Message was sent while issue was closed.
I'm afraid I have a post-landing not LGTM. Could you please do a follow-up to address my comment? Thanks! Vaclav https://codereview.chromium.org/2724153004/diff/40001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2724153004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:883: size_t limit; This is actually worse than before. Before there was not a single point in the code where |limit| would not be initialised. Now the declaration and assignment are separate, and if somebody adds code between them, there will be a non-trivial interval of |limit| having an undefined value. Please either revert to the previous state, or initialize limit. A good default value is SIZE_MAX. Note that you also still need to keep the assignment after calling StringToSizeT, because that method can modify |limit| even if it returns false. This would be good to capture in a comment here as well. size_t limit = SIZE_MAX;
Message was sent while issue was closed.
https://codereview.chromium.org/2724153004/diff/40001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2724153004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:883: size_t limit; On 2017/03/04 19:40:29, vabr (Chromium) wrote: > This is actually worse than before. Before there was not a single point in the > code where |limit| would not be initialised. Now the declaration and assignment > are separate, and if somebody adds code between them, there will be a > non-trivial interval of |limit| having an undefined value. > > Please either revert to the previous state, or initialize limit. A good default > value is SIZE_MAX. Note that you also still need to keep the assignment after > calling StringToSizeT, because that method can modify |limit| even if it returns > false. This would be good to capture in a comment here as well. > > size_t limit = SIZE_MAX; I will use another CL to fix this. Thanks!
Message was sent while issue was closed.
https://codereview.chromium.org/2724153004/diff/40001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2724153004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:883: size_t limit; On 2017/03/06 18:26:14, chengx wrote: > On 2017/03/04 19:40:29, vabr (Chromium) wrote: > > This is actually worse than before. Before there was not a single point in the > > code where |limit| would not be initialised. Now the declaration and > assignment > > are separate, and if somebody adds code between them, there will be a > > non-trivial interval of |limit| having an undefined value. > > > > Please either revert to the previous state, or initialize limit. A good > default > > value is SIZE_MAX. Note that you also still need to keep the assignment after > > calling StringToSizeT, because that method can modify |limit| even if it > returns > > false. This would be good to capture in a comment here as well. > > > > size_t limit = SIZE_MAX; > > I will use another CL to fix this. Thanks! SGTM, thanks for following up! :)
Message was sent while issue was closed.
On 2017/03/06 18:46:48, vabr (Chromium) wrote: > https://codereview.chromium.org/2724153004/diff/40001/components/autofill/cor... > File components/autofill/core/browser/personal_data_manager.cc (right): > > https://codereview.chromium.org/2724153004/diff/40001/components/autofill/cor... > components/autofill/core/browser/personal_data_manager.cc:883: size_t limit; > On 2017/03/06 18:26:14, chengx wrote: > > On 2017/03/04 19:40:29, vabr (Chromium) wrote: > > > This is actually worse than before. Before there was not a single point in > the > > > code where |limit| would not be initialised. Now the declaration and > > assignment > > > are separate, and if somebody adds code between them, there will be a > > > non-trivial interval of |limit| having an undefined value. > > > > > > Please either revert to the previous state, or initialize limit. A good > > default > > > value is SIZE_MAX. Note that you also still need to keep the assignment > after > > > calling StringToSizeT, because that method can modify |limit| even if it > > returns > > > false. This would be good to capture in a comment here as well. > > > > > > size_t limit = SIZE_MAX; > > > > I will use another CL to fix this. Thanks! > > SGTM, thanks for following up! :) LGTM because of the follow-up in https://codereview.chromium.org/2732913004/. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
