Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2378463002:
Remove unnecessary DCHECK in BucketOverlapsInterval(). (Closed)
|
Created:
4 years, 2 months ago by Not at Google. Contact bengr Modified:
4 years, 2 months ago Reviewers:
tbansal1 CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove unnecessary DCHECK in BucketOverlapsInterval().
BUG=650585
Committed: https://crrev.com/7d019e768800fe08d9391003702f46622e0bfdab
Cr-Commit-Position: refs/heads/master@{#421383}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Remove DCHECK only #Messages
Total messages: 21 (14 generated)
The CQ bit was checked by kundaji@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 ========== Support for null dates in DataUsageStore API. BUG=650585 ========== to ========== Support for null dates in DataUsageStore API. A null start date is interpreted as the beginning of time, and a null end date is interpreted as the end of time. BUG=650585 ==========
kundaji@chromium.org changed reviewers: + tbansal@chromium.org
https://codereview.chromium.org/2378463002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_usage_store.cc (left): https://codereview.chromium.org/2378463002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_usage_store.cc:228: DCHECK(!start_interval.is_null()); It seems that the only fix is to remove these 2 DCHECKs because |timestamp.is_null()| can legitimately be true. But, that should not affect any comparisons. https://codereview.chromium.org/2378463002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_usage_store.cc (right): https://codereview.chromium.org/2378463002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_usage_store.cc:165: DCHECK(start.is_null() || end.is_null() || start <= end); Can the |end| be null when |start| is not? https://codereview.chromium.org/2378463002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_usage_store.cc:187: start.is_null() || start < begin_history ? begin_history : start; Is this change required? If |start| is null, then it can't be greater than |begin_history|. In that case, existing code would have set |start_delete| to |begin_history|, which is same as the behavior in the modified code. |timestamp.is_null()| can legitimately be true, and there should not be a need for special code to handle that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2378463002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_usage_store.cc (left): https://codereview.chromium.org/2378463002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_usage_store.cc:228: DCHECK(!start_interval.is_null()); On 2016/09/27 20:49:41, tbansal1 wrote: > It seems that the only fix is to remove these 2 DCHECKs because > |timestamp.is_null()| can legitimately be true. But, that should not affect any > comparisons. Kept end_interval check as is as discussed. https://codereview.chromium.org/2378463002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_usage_store.cc (right): https://codereview.chromium.org/2378463002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_usage_store.cc:165: DCHECK(start.is_null() || end.is_null() || start <= end); On 2016/09/27 20:49:41, tbansal1 wrote: > Can the |end| be null when |start| is not? Yes. https://codereview.chromium.org/2378463002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_usage_store.cc:187: start.is_null() || start < begin_history ? begin_history : start; On 2016/09/27 20:49:41, tbansal1 wrote: > Is this change required? If |start| is null, then it can't be greater than > |begin_history|. In that case, existing code would have set |start_delete| to > |begin_history|, which is same as the behavior in the modified code. > > |timestamp.is_null()| can legitimately be true, and there should not be a need > for special code to handle that. Removed only the DCHECK and kept everything else as is.
The CQ bit was checked by kundaji@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...
lgtm % updating the stale cl description.
Description was changed from ========== Support for null dates in DataUsageStore API. A null start date is interpreted as the beginning of time, and a null end date is interpreted as the end of time. BUG=650585 ========== to ========== Remove unnecessary DCHECK in BucketOverlapsInterval. BUG=650585 ==========
Description was changed from ========== Remove unnecessary DCHECK in BucketOverlapsInterval. BUG=650585 ========== to ========== Remove unnecessary DCHECK in BucketOverlapsInterval(). BUG=650585 ==========
The CQ bit was unchecked by kundaji@chromium.org
The CQ bit was checked by kundaji@chromium.org
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 ========== Remove unnecessary DCHECK in BucketOverlapsInterval(). BUG=650585 ========== to ========== Remove unnecessary DCHECK in BucketOverlapsInterval(). BUG=650585 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove unnecessary DCHECK in BucketOverlapsInterval(). BUG=650585 ========== to ========== Remove unnecessary DCHECK in BucketOverlapsInterval(). BUG=650585 Committed: https://crrev.com/7d019e768800fe08d9391003702f46622e0bfdab Cr-Commit-Position: refs/heads/master@{#421383} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7d019e768800fe08d9391003702f46622e0bfdab Cr-Commit-Position: refs/heads/master@{#421383} |
