Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1833723002:
Initialize |bytes_so_far| local variable in SaveFileManager::SaveFinished. (Closed)
|
Created:
4 years, 9 months ago by Łukasz Anforowicz Modified:
4 years, 9 months ago Reviewers:
Randy Smith (Not in Mondays) CC:
chromium-reviews, asanka, darin-cc_chromium.org, rginda+watch_chromium.org, jam, daph_mail.ru Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitialize |bytes_so_far| local variable in SaveFileManager::SaveFinished.
As pointed out in https://crbug.com/594219#c29 the local variable
|bytes_so_far| is not initialized at the point of declaration. This is
okay in the current code (this variable *does* get intialized in all the
possible code flows), but the lack of initialization violates the style
guide [1] and is an unnecessary refactoring risk.
[1] https://google.github.io/styleguide/cppguide.html#Local_Variables says:
Place a function's variables in the narrowest scope possible, and
initialize variables in the declaration.
BUG=594219
Committed: https://crrev.com/34f48dde1eb15cdf568cba166aaf7db7f71a679e
Cr-Commit-Position: refs/heads/master@{#383163}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Remove re-initialization of |bytes_so_far|. #Messages
Total messages: 17 (9 generated)
Description was changed from ========== Initialize |bytes_so_far| local variable. As pointed out in https://crbug.com/594219#c29 the local variable |bytes_so_far| is not initialized at the point of declaration. This is okay in the current code (this variable *does* get intialized in all the possible code flows) lack of initialization violates the style guide and is an unnecessary refactoring risk. BUG=594219 ========== to ========== Initialize |bytes_so_far| local variable. As pointed out in https://crbug.com/594219#c29 the local variable |bytes_so_far| is not initialized at the point of declaration. This is okay in the current code (this variable *does* get intialized in all the possible code flows) lack of initialization violates the style guide [1] and is an unnecessary refactoring risk. [1] https://google.github.io/styleguide/cppguide.html#Local_Variables says: Place a function's variables in the narrowest scope possible, and initialize variables in the declaration. BUG=594219 ==========
Description was changed from ========== Initialize |bytes_so_far| local variable. As pointed out in https://crbug.com/594219#c29 the local variable |bytes_so_far| is not initialized at the point of declaration. This is okay in the current code (this variable *does* get intialized in all the possible code flows) lack of initialization violates the style guide [1] and is an unnecessary refactoring risk. [1] https://google.github.io/styleguide/cppguide.html#Local_Variables says: Place a function's variables in the narrowest scope possible, and initialize variables in the declaration. BUG=594219 ========== to ========== Initialize |bytes_so_far| local variable in SaveFileManager::SaveFinished. As pointed out in https://crbug.com/594219#c29 the local variable |bytes_so_far| is not initialized at the point of declaration. This is okay in the current code (this variable *does* get intialized in all the possible code flows), but the lack of initialization violates the style guide [1] and is an unnecessary refactoring risk. [1] https://google.github.io/styleguide/cppguide.html#Local_Variables says: Place a function's variables in the narrowest scope possible, and initialize variables in the declaration. BUG=594219 ==========
lukasza@chromium.org changed reviewers: + rdsmith@chromium.org
rdsmith@, could you please take a look? daph@, thanks for pointing out this issue. https://codereview.chromium.org/1833723002/diff/1/content/browser/download/sa... File content/browser/download/save_file_manager.cc (right): https://codereview.chromium.org/1833723002/diff/1/content/browser/download/sa... content/browser/download/save_file_manager.cc:209: int64_t bytes_so_far = 0; I think 0 is safe fallback here. Initializing to some non-sensical value (like -1 or int64-max-value) risks mishandling of the non-sense somewhere down the code (i.e. in functions receiving this value). PS. Before I was aware that the style guide mandates initialization everywhere, I thought that not initializing might be preferable, because 1) the compiler can catch it in simple cases [1] and 2) asan should catch it in more complex cases (and after this CL this won't longer be true - the variable will always be initialized to a "fallback" value). Do you think this makes sense? Does it need a broader discussion (i.e. on chromium-dev) or is it not really worth it?
I leave towards the rewrite I suggest below a bit more than "just an idea" but not strongly enough to gate review on it (partially because I think initializing the variable is the right thing to do (not doing so might mean we're more likely to catch bugs, but it also ups the chances of uninitialized memory showing up on users' machines), and partially because exactly how we structure the code doesn't strike me as very important). So LGTM, but do think about the idea. https://codereview.chromium.org/1833723002/diff/1/content/browser/download/sa... File content/browser/download/save_file_manager.cc (right): https://codereview.chromium.org/1833723002/diff/1/content/browser/download/sa... content/browser/download/save_file_manager.cc:209: int64_t bytes_so_far = 0; On 2016/03/24 18:23:28, Łukasz Anforowicz wrote: > I think 0 is safe fallback here. Initializing to some non-sensical value (like > -1 or int64-max-value) risks mishandling of the non-sense somewhere down the > code (i.e. in functions receiving this value). > > PS. Before I was aware that the style guide mandates initialization everywhere, > I thought that not initializing might be preferable, because 1) the compiler can > catch it in simple cases [1] and 2) asan should catch it in more complex cases > (and after this CL this won't longer be true - the variable will always be > initialized to a "fallback" value). Do you think this makes sense? Does it > need a broader discussion (i.e. on chromium-dev) or is it not really worth it? I don't think a broader discussion is worth it. This seems like a reasonable default; I'm a bit inclined to skip the re-assignment below, and just implement the semantic decision of "We got zero bytes unless the save file told us we got more than zero."
Thanks for reviewing. https://codereview.chromium.org/1833723002/diff/1/content/browser/download/sa... File content/browser/download/save_file_manager.cc (right): https://codereview.chromium.org/1833723002/diff/1/content/browser/download/sa... content/browser/download/save_file_manager.cc:209: int64_t bytes_so_far = 0; On 2016/03/24 19:17:49, Randy Smith - Not in Fridays wrote: > On 2016/03/24 18:23:28, Łukasz Anforowicz wrote: > > I think 0 is safe fallback here. Initializing to some non-sensical value > (like > > -1 or int64-max-value) risks mishandling of the non-sense somewhere down the > > code (i.e. in functions receiving this value). > > > > PS. Before I was aware that the style guide mandates initialization > everywhere, > > I thought that not initializing might be preferable, because 1) the compiler > can > > catch it in simple cases [1] and 2) asan should catch it in more complex cases > > (and after this CL this won't longer be true - the variable will always be > > initialized to a "fallback" value). Do you think this makes sense? Does it > > need a broader discussion (i.e. on chromium-dev) or is it not really worth it? > > I don't think a broader discussion is worth it. This seems like a reasonable > default; I'm a bit inclined to skip the re-assignment below, and just implement > the semantic decision of "We got zero bytes unless the save file told us we got > more than zero." Makes sense - done.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1833723002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833723002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1833723002/#ps20001 (title: "Remove re-initialization of |bytes_so_far|.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1833723002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833723002/20001
Message was sent while issue was closed.
Description was changed from ========== Initialize |bytes_so_far| local variable in SaveFileManager::SaveFinished. As pointed out in https://crbug.com/594219#c29 the local variable |bytes_so_far| is not initialized at the point of declaration. This is okay in the current code (this variable *does* get intialized in all the possible code flows), but the lack of initialization violates the style guide [1] and is an unnecessary refactoring risk. [1] https://google.github.io/styleguide/cppguide.html#Local_Variables says: Place a function's variables in the narrowest scope possible, and initialize variables in the declaration. BUG=594219 ========== to ========== Initialize |bytes_so_far| local variable in SaveFileManager::SaveFinished. As pointed out in https://crbug.com/594219#c29 the local variable |bytes_so_far| is not initialized at the point of declaration. This is okay in the current code (this variable *does* get intialized in all the possible code flows), but the lack of initialization violates the style guide [1] and is an unnecessary refactoring risk. [1] https://google.github.io/styleguide/cppguide.html#Local_Variables says: Place a function's variables in the narrowest scope possible, and initialize variables in the declaration. BUG=594219 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Initialize |bytes_so_far| local variable in SaveFileManager::SaveFinished. As pointed out in https://crbug.com/594219#c29 the local variable |bytes_so_far| is not initialized at the point of declaration. This is okay in the current code (this variable *does* get intialized in all the possible code flows), but the lack of initialization violates the style guide [1] and is an unnecessary refactoring risk. [1] https://google.github.io/styleguide/cppguide.html#Local_Variables says: Place a function's variables in the narrowest scope possible, and initialize variables in the declaration. BUG=594219 ========== to ========== Initialize |bytes_so_far| local variable in SaveFileManager::SaveFinished. As pointed out in https://crbug.com/594219#c29 the local variable |bytes_so_far| is not initialized at the point of declaration. This is okay in the current code (this variable *does* get intialized in all the possible code flows), but the lack of initialization violates the style guide [1] and is an unnecessary refactoring risk. [1] https://google.github.io/styleguide/cppguide.html#Local_Variables says: Place a function's variables in the narrowest scope possible, and initialize variables in the declaration. BUG=594219 Committed: https://crrev.com/34f48dde1eb15cdf568cba166aaf7db7f71a679e Cr-Commit-Position: refs/heads/master@{#383163} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/34f48dde1eb15cdf568cba166aaf7db7f71a679e Cr-Commit-Position: refs/heads/master@{#383163} |
