Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2015493002:
Mark the Izenpe log as Disqualified (Closed)
|
Created:
4 years, 7 months ago by Ryan Sleevi Modified:
4 years, 6 months ago CC:
chromium-reviews, certificate-transparency-chrome_googlegroups.com, cbentzel+watch_chromium.org, Eran Messeri Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMark the Izenpe log as Disqualified
The Izenpe Log should be disqualified as of 2016-05-30
BUG=614656
R=eroman@chromium.org
Committed: https://crrev.com/db47cd8c36459274d5634ae462059ee4a7e6434e
Cr-Commit-Position: refs/heads/master@{#396646}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Review feedback #
Messages
Total messages: 22 (6 generated)
Eric: Hopefully this addresses your previous concerns Eran: FYI
Oh, and the reason why the static TimeDelta is because it's now constexpr and thus not a static overhead. There's an additional int64 addition in the generated code, but that's not too bad (compared to using FromExploded, which is a syscall & platform dependent and grotty)
eranm@chromium.org changed reviewers: + eranm@chromium.org
lgtm from a policy perspective.
lgtm https://codereview.chromium.org/2015493002/diff/1/net/cert/ct_known_logs.cc File net/cert/ct_known_logs.cc (right): https://codereview.chromium.org/2015493002/diff/1/net/cert/ct_known_logs.cc#n... net/cert/ct_known_logs.cc:72: CHECK_EQ(log_id.size(), arraysize(kDisqualifiedCTLogList[0].log_id) - 1); If we don't already have it somewhere, would be prudent to have an assertion that the input data is sorted. A DCHECK() should be fine. (If something were to be added out of order it would break searching) https://codereview.chromium.org/2015493002/diff/1/net/cert/ct_known_logs_stat... File net/cert/ct_known_logs_static-inc.h (right): https://codereview.chromium.org/2015493002/diff/1/net/cert/ct_known_logs_stat... net/cert/ct_known_logs_static-inc.h:93: const base::TimeDelta disqualification_date; Why the change? https://codereview.chromium.org/2015493002/diff/1/net/cert/ct_known_logs_stat... net/cert/ct_known_logs_static-inc.h:122: base::TimeDelta::FromSeconds(INT64_C(1460678400)), You can drop the INT64_C() now
https://codereview.chromium.org/2015493002/diff/1/net/cert/ct_known_logs_stat... File net/cert/ct_known_logs_static-inc.h (right): https://codereview.chromium.org/2015493002/diff/1/net/cert/ct_known_logs_stat... net/cert/ct_known_logs_static-inc.h:93: const base::TimeDelta disqualification_date; On 2016/05/26 00:47:40, eroman wrote: > Why the change? Already answered this in the previous message, but it seems to have been missed 1) You rightly raised a concern about FromInternalValue() in the previous CL, and expressed a desire for something more readable. FromExploded has issues, but we can at least use the Unix Epoch as a point 2) TimeDelta is now constexpr, so this no longer invokes a static initializer, therefore it's semantically clearer than int64_t https://codereview.chromium.org/2015493002/diff/1/net/cert/ct_known_logs_stat... net/cert/ct_known_logs_static-inc.h:122: base::TimeDelta::FromSeconds(INT64_C(1460678400)), On 2016/05/26 00:47:40, eroman wrote: > You can drop the INT64_C() now Are you asking me to remove this? INT64_C is part of stdint, it's part of C++11, and it's part of general portable code. It would seem you're saying "You don't need to write portable code" - which, while true, is unclear if you're also saying "You shouldn't write portable code"
https://codereview.chromium.org/2015493002/diff/1/net/cert/ct_known_logs_stat... File net/cert/ct_known_logs_static-inc.h (right): https://codereview.chromium.org/2015493002/diff/1/net/cert/ct_known_logs_stat... net/cert/ct_known_logs_static-inc.h:122: base::TimeDelta::FromSeconds(INT64_C(1460678400)), On 2016/05/26 08:49:50, Ryan Sleevi (OOO til 6-6) wrote: > On 2016/05/26 00:47:40, eroman wrote: > > You can drop the INT64_C() now > > Are you asking me to remove this? > > INT64_C is part of stdint, it's part of C++11, and it's part of general portable > code. It would seem you're saying "You don't need to write portable code" - > which, while true, is unclear if you're also saying "You shouldn't write > portable code" Yes, I was asking you to remove it. Do you really find: base::TimeDelta::FromSeconds(INT64_C(1460678400)) To be more readable than: base::TimeDelta::FromSeconds(1460678400), ?
On 2016/05/26 19:30:16, eroman wrote: > Do you really find: > base::TimeDelta::FromSeconds(INT64_C(1460678400)) > > To be more readable than: > base::TimeDelta::FromSeconds(1460678400), > > ? That isn't what I was talking about. I disagree with your line of reasoning, and it's not compliant with the C++ spec's guarantees (aka it's implementation dependent), but it's not worth arguing about - other than I view writing 'correct' and 'portable' code to be more important.
> That isn't what I was talking about. I disagree with your line of reasoning, and > it's not compliant with the C++ spec's guarantees (aka it's implementation > dependent), but it's not worth arguing about - other than I view writing > 'correct' and 'portable' code to be more important. What is your portability concern? My suggestion is to replace a |INT64_C(xxx)| with simply |xxx|.... We don't write for instance: base::TimeDuration::FromSeconds(INT64_C(1)) We just write: base::TimeDuration::FromSeconds(1) Because an int can be safely promoted to an int64_t, and there is no truncation happening to the int literal. In this case the integer literal (seconds since unique epoch) can be safely expressed as just an int without truncation, so no need to suffix it. (Previously when it was an internal value that was not the case) The only portability issue would be if ints are smaller than 16 bits. But that is not an assumption Chrome has ever made, and would be catastrophic all over the place if attempted. Certainly not worth arguing over (I already LGTMed), but I did want to follow-up with you to understand your objection in case we are talking about different things.
"smaller than 16 bits." -- er I meant 32-bits obviously :)
On 2016/05/26 20:19:00, eroman wrote: > Because an int can be safely promoted to an int64_t, and there is no truncation > happening to the int literal. Counting fail on my part on the size of my number (was thinking we had an implicit long-long-int to int64_t conversion, since long int is only assured to 32 bits, but long-long-int isn't assured to be larger than long-int or equal-to int64_t).
I opted to make it a unittest, rather than a DCHECK, so that there isn't unnecessary runtime overhead for people running with DCHECKs enabled. Similarly, I'm sure it could be made into some constexpr with some cleverness, but that just means everyone has to pay the cost at compile-time (plus, who wants to read my template hell) Hopefully that's sufficient for you, but relying on the previous LGTM since it's a trivial change.
The CQ bit was checked by rsleevi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org, eranm@chromium.org Link to the patchset: https://codereview.chromium.org/2015493002/#ps20001 (title: "Review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2015493002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2015493002/20001
The CQ bit was unchecked by rsleevi@chromium.org
lgtm for the new unittest, thanks!
The CQ bit was checked by rsleevi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2015493002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2015493002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Mark the Izenpe log as Disqualified The Izenpe Log should be disqualified as of 2016-05-30 BUG=614656 R=eroman@chromium.org ========== to ========== Mark the Izenpe log as Disqualified The Izenpe Log should be disqualified as of 2016-05-30 BUG=614656 R=eroman@chromium.org Committed: https://crrev.com/db47cd8c36459274d5634ae462059ee4a7e6434e Cr-Commit-Position: refs/heads/master@{#396646} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/db47cd8c36459274d5634ae462059ee4a7e6434e Cr-Commit-Position: refs/heads/master@{#396646} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
