Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10540082:
Coverity: Remove dead code. (Closed)
|
Created:
8 years, 6 months ago by kmadhusu Modified:
8 years, 6 months ago CC:
chromium-reviews, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCoverity: Remove dead code.
CID=104183
BUG=none
TEST=none
TBR=sky@chromium.org,mpearson@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141698
Patch Set 1 #
Total comments: 2
Messages
Total messages: 9 (0 generated)
https://chromiumcodereview.appspot.com/10540082/diff/1/chrome/browser/history... File chrome/browser/history/scored_history_match.cc (left): https://chromiumcodereview.appspot.com/10540082/diff/1/chrome/browser/history... chrome/browser/history/scored_history_match.cc:454: unnormalized_recency_score = 10; Since days_ago is always between 0 and 365, execution cannot reach this statement.
+ mpearson LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10540082/1
Change committed as 141698
https://chromiumcodereview.appspot.com/10540082/diff/1/chrome/browser/history... File chrome/browser/history/scored_history_match.cc (left): https://chromiumcodereview.appspot.com/10540082/diff/1/chrome/browser/history... chrome/browser/history/scored_history_match.cc:454: unnormalized_recency_score = 10; On 2012/06/08 19:51:14, kmadhusu wrote: > Since days_ago is always between 0 and 365, execution cannot reach this > statement. Hmm I looked at this Coverity defect, and had reservations. This change relies on the fact that kDaysToPrecomputeRecencyScoresFor is 365, which this code should not know about. I don't know what the best solution is (maybe this is the best solution).
On 2012/06/12 20:38:57, James Hawkins wrote: > https://chromiumcodereview.appspot.com/10540082/diff/1/chrome/browser/history... > File chrome/browser/history/scored_history_match.cc (left): > > https://chromiumcodereview.appspot.com/10540082/diff/1/chrome/browser/history... > chrome/browser/history/scored_history_match.cc:454: unnormalized_recency_score = > 10; > On 2012/06/08 19:51:14, kmadhusu wrote: > > Since days_ago is always between 0 and 365, execution cannot reach this > > statement. > > Hmm I looked at this Coverity defect, and had reservations. This change relies > on the fact that kDaysToPrecomputeRecencyScoresFor is 365, which this code > should not know about. I don't know what the best solution is (maybe this is > the best solution). FWIW, I had reservations about this too. I had more reservations about the other change Coverity found on this same changelist I submitted. Here's Coverity's other fix: http://codereview.chromium.org/10535084/ I think removing this test is a bit misleading about how I think the function should behave if the compile time constant kMaxRawTermScore is increased. I didn't say anything at the time because it didn't want to spend time arguing about it. Now that someone else said something, I thought I should speak up. If the compiler can dead-code-remove-them, then what's the harm in having more clear, defensible code for other readers the code? --mark
On 2012/06/12 21:16:11, Mark P wrote: > On 2012/06/12 20:38:57, James Hawkins wrote: > > > https://chromiumcodereview.appspot.com/10540082/diff/1/chrome/browser/history... > > File chrome/browser/history/scored_history_match.cc (left): > > > > > https://chromiumcodereview.appspot.com/10540082/diff/1/chrome/browser/history... > > chrome/browser/history/scored_history_match.cc:454: unnormalized_recency_score > = > > 10; > > On 2012/06/08 19:51:14, kmadhusu wrote: > > > Since days_ago is always between 0 and 365, execution cannot reach this > > > statement. > > > > Hmm I looked at this Coverity defect, and had reservations. This change > relies > > on the fact that kDaysToPrecomputeRecencyScoresFor is 365, which this code > > should not know about. I don't know what the best solution is (maybe this is > > the best solution). > > FWIW, I had reservations about this too. I had more reservations about the > other change Coverity found on this same changelist I submitted. Here's > Coverity's other fix: > http://codereview.chromium.org/10535084/ > > I think removing this test is a bit misleading about how I think the function > should behave if the compile time constant kMaxRawTermScore is increased. > > I didn't say anything at the time because it didn't want to spend time arguing > about it. Now that someone else said something, I thought I should speak up. > If the compiler can dead-code-remove-them, then what's the harm in having more > clear, defensible code for other readers the code? > > --mark mpearson@: I don't have any strong opinion. If you think the else block is intentional, I can revert my CL's. Generally, if I want to write a defensive code, I will write an else block with a NOTREACHED() statment rather than an actual expression/statement. That's why I thought its okay to delete this logically structured dead code for the given constant values. May be in future, please write a comment stating that the else block is intentional. It will be helpful to mark the coverity bug as intentional. jhawkins@: If you saw some defects and if you had some reservations, please leave a comment in the bug or mark as intentional. It will help the future developers to not to work on that issue. Thanks.
On 2012/06/12 22:02:11, kmadhusu wrote: > On 2012/06/12 21:16:11, Mark P wrote: > > On 2012/06/12 20:38:57, James Hawkins wrote: > > > > > > https://chromiumcodereview.appspot.com/10540082/diff/1/chrome/browser/history... > > > File chrome/browser/history/scored_history_match.cc (left): > > > > > > > > > https://chromiumcodereview.appspot.com/10540082/diff/1/chrome/browser/history... > > > chrome/browser/history/scored_history_match.cc:454: > unnormalized_recency_score > > = > > > 10; > > > On 2012/06/08 19:51:14, kmadhusu wrote: > > > > Since days_ago is always between 0 and 365, execution cannot reach this > > > > statement. > > > > > > Hmm I looked at this Coverity defect, and had reservations. This change > > relies > > > on the fact that kDaysToPrecomputeRecencyScoresFor is 365, which this code > > > should not know about. I don't know what the best solution is (maybe this > is > > > the best solution). > > > > FWIW, I had reservations about this too. I had more reservations about the > > other change Coverity found on this same changelist I submitted. Here's > > Coverity's other fix: > > http://codereview.chromium.org/10535084/ > > > > I think removing this test is a bit misleading about how I think the function > > should behave if the compile time constant kMaxRawTermScore is increased. > > > > I didn't say anything at the time because it didn't want to spend time arguing > > about it. Now that someone else said something, I thought I should speak up. > > If the compiler can dead-code-remove-them, then what's the harm in having more > > clear, defensible code for other readers the code? > > > > --mark > > mpearson@: I don't have any strong opinion. If you think the else block is > intentional, I can revert my CL's. Generally, if I want to write a defensive > code, I will write an else block with a NOTREACHED() statment rather than an > actual expression/statement. That's why I thought its okay to delete this > logically structured dead code for the given constant values. May be in future, > please write a comment stating that the else block is intentional. It will be > helpful to mark the coverity bug as intentional. > > jhawkins@: If you saw some defects and if you had some reservations, please > leave a comment in the bug or mark as intentional. It will help the future > developers to not to work on that issue. > The bug was marked assigned (by you), so I figured you came up with the solution that was alluding me.
On 2012/06/18 17:49:44, James Hawkins wrote: > On 2012/06/12 22:02:11, kmadhusu wrote: > > On 2012/06/12 21:16:11, Mark P wrote: > > > On 2012/06/12 20:38:57, James Hawkins wrote: > > > > > > > > > > https://chromiumcodereview.appspot.com/10540082/diff/1/chrome/browser/history... > > > > File chrome/browser/history/scored_history_match.cc (left): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10540082/diff/1/chrome/browser/history... > > > > chrome/browser/history/scored_history_match.cc:454: > > unnormalized_recency_score > > > = > > > > 10; > > > > On 2012/06/08 19:51:14, kmadhusu wrote: > > > > > Since days_ago is always between 0 and 365, execution cannot reach this > > > > > statement. > > > > > > > > Hmm I looked at this Coverity defect, and had reservations. This change > > > relies > > > > on the fact that kDaysToPrecomputeRecencyScoresFor is 365, which this code > > > > should not know about. I don't know what the best solution is (maybe this > > is > > > > the best solution). > > > > > > FWIW, I had reservations about this too. I had more reservations about the > > > other change Coverity found on this same changelist I submitted. Here's > > > Coverity's other fix: > > > http://codereview.chromium.org/10535084/ > > > > > > I think removing this test is a bit misleading about how I think the > function > > > should behave if the compile time constant kMaxRawTermScore is increased. > > > > > > I didn't say anything at the time because it didn't want to spend time > arguing > > > about it. Now that someone else said something, I thought I should speak > up. > > > If the compiler can dead-code-remove-them, then what's the harm in having > more > > > clear, defensible code for other readers the code? > > > > > > --mark > > > > mpearson@: I don't have any strong opinion. If you think the else block is > > intentional, I can revert my CL's. Generally, if I want to write a defensive > > code, I will write an else block with a NOTREACHED() statment rather than an > > actual expression/statement. That's why I thought its okay to delete this > > logically structured dead code for the given constant values. May be in > future, > > please write a comment stating that the else block is intentional. It will be > > helpful to mark the coverity bug as intentional. > > > > jhawkins@: If you saw some defects and if you had some reservations, please > > leave a comment in the bug or mark as intentional. It will help the future > > developers to not to work on that issue. > > > > The bug was marked assigned (by you), so I figured you came up with the solution > that was alluding me. jhawkins: I thought you investigated the bug before I started to work on it. That's why I asked you to add more details if you had some reservations. Sorry for the confusion. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
