Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1988663002:
Add: check exploded time is properly converted (Closed)
|
Created:
4 years, 7 months ago by maksims (do not use this acc) Modified:
4 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd: check exploded time is properly converted.
This cl introduces time checking in posix-like and mac systems.
base::Time::FromUTCExploded() and
base::Time::FromLocalExplode() can fail without returning
a proper error.
This fix does the following:
1) After calculations are done, create UTC or local time.
2) Convert UTC or local time back to exploded
3) Compare original exploded with converted one
4) If times are not equal, then return Time(0) indicating
an error.
Windows implementation already returns Time(0) on error.
BUG=601900
Committed: https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382
Committed: https://crrev.com/558f168054566ecbc611918093398add04dc13a4
Cr-Original-Commit-Position: refs/heads/master@{#396638}
Cr-Commit-Position: refs/heads/master@{#399794}
Patch Set 1 #Patch Set 2 : handling local and utc times + unittests #
Total comments: 25
Patch Set 3 : overloaded FromUTCExplode and FromLocalExplode for transition process if accepted #Patch Set 4 : #
Total comments: 22
Patch Set 5 : mmenke comments #
Total comments: 16
Patch Set 6 : #
Total comments: 5
Patch Set 7 : #
Messages
Total messages: 63 (25 generated)
Description was changed from ========== adding time check BUG= ========== to ========== Add, Posix: check exploded time is properly converted to UTC This cl introduces time checking in posix-like systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC time. 2) Convert UTC time back to exploded 3) Compare original exploded with converted one 4) If !=, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 ==========
maksim.sisov@intel.com changed reviewers: + mark@chromium.org, mmenke@chromium.org
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
maksim.sisov@intel.com changed reviewers: + eroman@chromium.org
Would you please take a look?
Description was changed from ========== Add, Posix: check exploded time is properly converted to UTC This cl introduces time checking in posix-like systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC time. 2) Convert UTC time back to exploded 3) Compare original exploded with converted one 4) If !=, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 ========== to ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If !=, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 ==========
This looks very reasonable to me. https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h#newcode1 base/time/time.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. You shouldn't update copyright dates when updating files. We used to do that, but then the word came not to do it. I'm not a lawyer, so can't explain why. This goes for all files. https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc#n... base/time/time_mac.cc:193: // on a 28-30 day month, it will return the first day of the next month. We should figure out what platforms this happens on, and only have this extra logic on those platforms. How do we figure that out? Simplest thing to do would be for you to upload a CL with your unit test, but without these fixes, and I'll run it through the trybots, and we can see which ones fail. https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc#n... base/time/time_mac.cc:197: // Windows and Posix implementations return Time(0) on failure. These last two sentences seem not to be needed. https://codereview.chromium.org/1988663002/diff/60001/base/time/time_unittest.cc File base/time/time_unittest.cc (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time_unittest... base/time/time_unittest.cc:128: TEST_F(TimeTest, FromExplodedError) { Maybe call this test "FromExplodedOutOfBoundsTime"? https://codereview.chromium.org/1988663002/diff/60001/base/time/time_unittest... base/time/time_unittest.cc:129: // FromUTCExploded might return Time(0), if the day is set to 31 on a nit: might -> must
https://codereview.chromium.org/1988663002/diff/60001/base/time/time.cc File base/time/time.cc (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time.cc#newco... base/time/time.cc:348: return (month != rhs.month || day_of_month != rhs.day_of_month || year? https://codereview.chromium.org/1988663002/diff/60001/base/time/time.cc#newco... base/time/time.cc:350: millisecond != rhs.millisecond); The outer (parentheses) surround the entire expression and aren’t necessary. https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc#n... base/time/time_mac.cc:198: base::Time::Exploded to_exploded = {0}; No need to waste time {0}-initializing.
https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h#newcod... base/time/time.h:423: bool operator!=(const Exploded& rhs); I don't think we want to call this operator!=(). The issue is that with a == or != one expects every field to be compared for equality. Whereas for this application, we do *not* want to compare equality of |day_of_week|, since that is a computed value. Other naming ideas: IsEquivalent() Matches() https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h#newcod... base/time/time.h:518: // into a Time class. This documentation should also be updated to clarify what happens on errors, and what is considered an error. While this is definitely an improvement over the current situation, (and I am on board with making this change) I do feel that an explicit error condition would be better (i.e. return a boolean and assign to an Exploded). With a sentinel value based approach like Time(0) there is the usual problem of ambiguity arising as to whether Time(0) means the earliest possible time, or an error. The following is a contrived example, but consider the case of certificate verification -- if we parse the "notBefore" and get Time(0) we would have to treat that as meaning parsing failed and reject the certificate. Whereas it may have been the notBefore truly was Time(0) and parsing succeeded, and the certificate is subsequently valid. ... Admittedly this is kind of a bogus example, since Time(0) is 1601, and I am pretty sure our posix time exploding implementations can't go earlier than 1970. But maybe this sequence of events would happen on Windows. https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc#n... base/time/time_mac.cc:193: // on a 28-30 day month, it will return the first day of the next month. On 2016/05/18 17:32:00, mmenke wrote: > We should figure out what platforms this happens on, and only have this extra > logic on those platforms. How do we figure that out? Simplest thing to do > would be for you to upload a CL with your unit test, but without these fixes, > and I'll run it through the trybots, and we can see which ones fail. In the case of our _posix implementation, the specification for mktime() is surprisingly ambiguous on this issue. In practice though you can pass out of range values and it will do the arithmetic (for instance Feb -1 becomes Jan 31). This is a notable difference from the Windows implementation. So yeah, matching the lowest-common denominator here is good. https://codereview.chromium.org/1988663002/diff/60001/base/time/time_unittest.cc File base/time/time_unittest.cc (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time_unittest... base/time/time_unittest.cc:152: exploded.minute = 30; Good start, but will also want tests for: * negative months/days/hours/minutes/seconds * seconds/minutes/hours/milliseconds that are too large
https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h#newcod... base/time/time.h:423: bool operator!=(const Exploded& rhs); On 2016/05/18 19:35:17, eroman wrote: > I don't think we want to call this operator!=(). > > The issue is that with a == or != one expects every field to be compared for > equality. > > Whereas for this application, we do *not* want to compare equality of > |day_of_week|, since that is a computed value. > > Other naming ideas: > > IsEquivalent() > Matches() I had the same comment as a draft, then I went to recheck what the style guide said to get a good link to it, and I found that the recommendation has changed. https://google.github.io/styleguide/cppguide.html#Operator_Overloading Notably: > Don't go out of your way to avoid defining operator overloads. For example, > prefer to define ==, =, and <<, rather than Equals(), CopyFrom(), and > PrintTo(). So I’m OK with this overload now.
https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc#n... base/time/time_mac.cc:193: // on a 28-30 day month, it will return the first day of the next month. On 2016/05/18 19:35:17, eroman wrote: > On 2016/05/18 17:32:00, mmenke wrote: > > We should figure out what platforms this happens on, and only have this extra > > logic on those platforms. How do we figure that out? Simplest thing to do > > would be for you to upload a CL with your unit test, but without these fixes, > > and I'll run it through the trybots, and we can see which ones fail. > > In the case of our _posix implementation, the specification for mktime() is > surprisingly ambiguous on this issue. > > In practice though you can pass out of range values and it will do the > arithmetic (for instance Feb -1 becomes Jan 31). > > This is a notable difference from the Windows implementation. > > So yeah, matching the lowest-common denominator here is good. That's a good point...I was mostly thinking (hoping?) OSX and Windows would at least either consistently be good or consistently be bad...but that may be hoping for too much. Having duplicate code on each platform to check the time seems ugly, though not sure it's worth the effort of avoiding.
An alternative would be to have the unexploder match the highest common denominator on platforms where it doesn’t naturally do so. In other words, if it’s convenient to be able to say “March 0” to get the last day of February, do fixups on Windows to make that happen. As long as you’re sure that the unexploded representation assumes 60-second minutes and you hard-code the right rules for the number of days in different Februaries, this is doable by fixing up the exploded time that you feed to the system’s unexploder. But in reality, none of our code can depend on this behavior now if the unaided system unexploder on Windows provides it. Unless there are call sites that really want to be able to do this now, forcing the lowest common denominator seems like the right move.
On 2016/05/18 20:16:47, Mark Mentovai wrote: > An alternative would be to have the unexploder match the highest common > denominator on platforms where it doesn’t naturally do so. In other words, if > it’s convenient to be able to say “March 0” to get the last day of February, do > fixups on Windows to make that happen. The main reason this came up is this is used for data received over the network, where we really don't want to extend APIs to support things like "March 0". Admittedly, all components receiving times from the network could do their own checks, but I'm not sure that really scales. > As long as you’re sure that the unexploded representation assumes 60-second > minutes and you hard-code the right rules for the number of days in different > Februaries, this is doable by fixing up the exploded time that you feed to the > system’s unexploder. > > But in reality, none of our code can depend on this behavior now if the unaided > system unexploder on Windows provides it. Unless there are call sites that > really want to be able to do this now, forcing the lowest common denominator > seems like the right move.
mmenke wrote: > On 2016/05/18 20:16:47, Mark Mentovai wrote: > > An alternative would be to have the unexploder match the highest common > > denominator on platforms where it doesn’t naturally do so. In other words, if > > it’s convenient to be able to say “March 0” to get the last day of February, > do > > fixups on Windows to make that happen. > > The main reason this came up is this is used for data received over the network, > where we really don't want to extend APIs to support things like "March 0". > Admittedly, all components receiving times from the network could do their own > checks, but I'm not sure that really scales. We could centralize the check (FromExplodedStrict or something), but like I said before, if we don’t have anyone that wants to not be strict about this today, we might as well be strict for everyone rather than force call sites to choose.
Patchset #3 (id:80001) has been deleted
Would you please take a look? As eroman proposed, I made FromUTCExplode and FromLocalExplode to return true or false on failure and set corresponding |time|. These overloaded functions will be a transition to a newer form. mmenke@, would you please run the trybots? https://codereview.chromium.org/1988663002/diff/60001/base/time/time.cc File base/time/time.cc (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time.cc#newco... base/time/time.cc:348: return (month != rhs.month || day_of_month != rhs.day_of_month || On 2016/05/18 17:42:49, Mark Mentovai wrote: > year? Done. https://codereview.chromium.org/1988663002/diff/60001/base/time/time.cc#newco... base/time/time.cc:350: millisecond != rhs.millisecond); On 2016/05/18 17:42:49, Mark Mentovai wrote: > The outer (parentheses) surround the entire expression and aren’t necessary. Done. https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h#newcode1 base/time/time.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/05/18 17:32:00, mmenke wrote: > You shouldn't update copyright dates when updating files. We used to do that, > but then the word came not to do it. I'm not a lawyer, so can't explain why. > > This goes for all files. Done. https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h#newcod... base/time/time.h:423: bool operator!=(const Exploded& rhs); On 2016/05/18 20:07:15, Mark Mentovai wrote: > On 2016/05/18 19:35:17, eroman wrote: > > I don't think we want to call this operator!=(). > > > > The issue is that with a == or != one expects every field to be compared for > > equality. > > > > Whereas for this application, we do *not* want to compare equality of > > |day_of_week|, since that is a computed value. > > > > Other naming ideas: > > > > IsEquivalent() > > Matches() > > I had the same comment as a draft, then I went to recheck what the style guide > said to get a good link to it, and I found that the recommendation has changed. > > https://google.github.io/styleguide/cppguide.html#Operator_Overloading > > Notably: > > > Don't go out of your way to avoid defining operator overloads. For example, > > prefer to define ==, =, and <<, rather than Equals(), CopyFrom(), and > > PrintTo(). > > So I’m OK with this overload now. Ok! https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h#newcod... base/time/time.h:518: // into a Time class. On 2016/05/18 19:35:17, eroman wrote: > This documentation should also be updated to clarify what happens on errors, and > what is considered an error. > > While this is definitely an improvement over the current situation, (and I am on > board with making this change) I do feel that an explicit error condition would > be better (i.e. return a boolean and assign to an Exploded). > > With a sentinel value based approach like Time(0) there is the usual problem of > ambiguity arising as to whether Time(0) means the earliest possible time, or an > error. > > The following is a contrived example, but consider the case of certificate > verification -- if we parse the "notBefore" and get Time(0) we would have to > treat that as meaning parsing failed and reject the certificate. Whereas it may > have been the notBefore truly was Time(0) and parsing succeeded, and the > certificate is subsequently valid. ... Admittedly this is kind of a bogus > example, since Time(0) is 1601, and I am pretty sure our posix time exploding > implementations can't go earlier than 1970. But maybe this sequence of events > would happen on Windows. Well, can you please check the patch? Your proposal sounds very reasonable and in order to transform other code, I overloaded the functions to provide callers with true or false and the converted time. https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc#n... base/time/time_mac.cc:193: // on a 28-30 day month, it will return the first day of the next month. On 2016/05/18 20:11:32, mmenke wrote: > On 2016/05/18 19:35:17, eroman wrote: > > On 2016/05/18 17:32:00, mmenke wrote: > > > We should figure out what platforms this happens on, and only have this > extra > > > logic on those platforms. How do we figure that out? Simplest thing to do > > > would be for you to upload a CL with your unit test, but without these > fixes, > > > and I'll run it through the trybots, and we can see which ones fail. > > > > In the case of our _posix implementation, the specification for mktime() is > > surprisingly ambiguous on this issue. > > > > In practice though you can pass out of range values and it will do the > > arithmetic (for instance Feb -1 becomes Jan 31). > > > > This is a notable difference from the Windows implementation. > > > > So yeah, matching the lowest-common denominator here is good. > > That's a good point...I was mostly thinking (hoping?) OSX and Windows would at > least either consistently be good or consistently be bad...but that may be > hoping for too much. > > Having duplicate code on each platform to check the time seems ugly, though not > sure it's worth the effort of avoiding. What's then? Can you run trybots to see what environments fail to execute FromExplodedOutOfBoundsTimeOld? https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc#n... base/time/time_mac.cc:197: // Windows and Posix implementations return Time(0) on failure. On 2016/05/18 17:32:00, mmenke wrote: > These last two sentences seem not to be needed. Done. https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc#n... base/time/time_mac.cc:198: base::Time::Exploded to_exploded = {0}; On 2016/05/18 17:42:49, Mark Mentovai wrote: > No need to waste time {0}-initializing. Done. https://codereview.chromium.org/1988663002/diff/60001/base/time/time_unittest.cc File base/time/time_unittest.cc (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time_unittest... base/time/time_unittest.cc:128: TEST_F(TimeTest, FromExplodedError) { On 2016/05/18 17:32:01, mmenke wrote: > Maybe call this test "FromExplodedOutOfBoundsTime"? Done. https://codereview.chromium.org/1988663002/diff/60001/base/time/time_unittest... base/time/time_unittest.cc:129: // FromUTCExploded might return Time(0), if the day is set to 31 on a On 2016/05/18 17:32:01, mmenke wrote: > nit: might -> must Done. https://codereview.chromium.org/1988663002/diff/60001/base/time/time_unittest... base/time/time_unittest.cc:152: exploded.minute = 30; On 2016/05/18 19:35:17, eroman wrote: > Good start, but will also want tests for: > * negative months/days/hours/minutes/seconds > * seconds/minutes/hours/milliseconds that are too large Done.
The CQ bit was checked by mmenke@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/1988663002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988663002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hrm, looks like everywhere but Windows has the same behavior here.
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Windows x86 and x64 hit NOTREACHED() on failure and [5516:7412:0520/014754:23213650:FATAL:time_win.cc(295)] Check failed: false. Unable to convert time (--- this is for old) and [432:6764:0520/014742:23201716:FATAL:time_win.cc(263)] Check failed: false. Unable to convert time (--- this is for new) is thrown that cause both TimeTestOutOfBounds.FromExplodedOutOfBoundsTimeOld and TimeTestOutOfBounds.FromExplodedOutOfBoundsTimeNew to fail. Is it an expected behavior? What's the reason of having return Time(0) there if it crashes on NOTREACHED()?
On 2016/05/20 10:18:48, maksims wrote: > Windows x86 and x64 hit NOTREACHED() on failure and > [5516:7412:0520/014754:23213650:FATAL:time_win.cc(295)] Check failed: false. > Unable to convert time (--- this is for old) and > [432:6764:0520/014742:23201716:FATAL:time_win.cc(263)] Check failed: false. > Unable to convert time (--- this is for new) > is thrown that cause both TimeTestOutOfBounds.FromExplodedOutOfBoundsTimeOld and > TimeTestOutOfBounds.FromExplodedOutOfBoundsTimeNew to fail. > > Is it an expected behavior? What's the reason of having return Time(0) there if > it crashes on NOTREACHED()? > [5516:7412:0520/014754:23213650:FATAL:time_win.cc(295)] Check failed: false. > Unable to convert time (--- this is for NEW) and > [432:6764:0520/014742:23201716:FATAL:time_win.cc(263)] Check failed: false. > Unable to convert time (--- this is for OLD) other way round...
https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h#newco... base/time/time.h:533: static bool FromUTCExploded(const Exploded& exploded, Time& time) { Per Google C++ style guide, non-const refs are forbidden. These should be passing in a Time*. https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h#newco... base/time/time.h:536: static bool FromLocalExploded(const Exploded& exploded, Time& time) { These two should use WARN_UNUSED_RESULT. https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h#newco... base/time/time.h:538: } Is the plan to replace calls to the old methods with calls to these? https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h#newco... base/time/time.h:579: static Time FromExploded(bool is_local, const Exploded& exploded); If we're going to keep the old methods around (Permanently? Temporarily?), we should probably still get rid of this method, and make the two old public functions wrap the new FromExploded function. Also, since the new methods have WARN_UNUSED_RESULT in the new methods, you'll need to use ignore_result on the calls in the two old methods to the new FromExploded method. https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h#newco... base/time/time.h:586: static bool FromExploded(bool is_local, const Exploded& exploded, Time& time); WARN_UNUSED_RESULT https://codereview.chromium.org/1988663002/diff/160001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1988663002/diff/160001/base/time/time_mac.cc#... base/time/time_mac.cc:188: base::Time t_time = nit: t_time seems like a weird variable name - it's not clear what the "t_" means. I'd suggest just calling it "time". https://codereview.chromium.org/1988663002/diff/160001/base/time/time_mac.cc#... base/time/time_mac.cc:195: // |utc_to_exploded| time. nit: Comment can be reformatted - first line of text is unusually short. https://codereview.chromium.org/1988663002/diff/160001/base/time/time_posix.cc File base/time/time_posix.cc (right): https://codereview.chromium.org/1988663002/diff/160001/base/time/time_posix.c... base/time/time_posix.cc:305: kWindowsEpochDeltaMicroseconds); Again, suggest just calling this |time|, unless there's another variable with that name I'm missing. In which case, could call it something like out, result, out_time, etc. https://codereview.chromium.org/1988663002/diff/160001/base/time/time_posix.c... base/time/time_posix.cc:318: return to_exploded != exploded ? Time(0) : t_time; nit: Fix indent. You can automatically format everything your CL touches with "git cl format". https://codereview.chromium.org/1988663002/diff/160001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/1988663002/diff/160001/base/time/time_win.cc#... base/time/time_win.cc:288: success = TzSpecificLocalTimeToSystemTime(NULL, &st, &utc_st) && nit: nullptr is now preferred over NULL. https://codereview.chromium.org/1988663002/diff/160001/base/time/time_win.cc#... base/time/time_win.cc:295: NOTREACHED() << "Unable to convert time"; Looks like this NOTREACHED is incorrect (In the old code, too). We just never tested it with invalid times, and it's rare enough to see them, that we never ran into the issue.
Patchset #6 (id:200001) has been deleted
Patchset #5 (id:180001) has been deleted
please take a look https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h#newco... base/time/time.h:533: static bool FromUTCExploded(const Exploded& exploded, Time& time) { On 2016/05/20 19:44:56, mmenke wrote: > Per Google C++ style guide, non-const refs are forbidden. These should be > passing in a Time*. Done. https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h#newco... base/time/time.h:536: static bool FromLocalExploded(const Exploded& exploded, Time& time) { On 2016/05/20 19:44:56, mmenke wrote: > These two should use WARN_UNUSED_RESULT. Done. https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h#newco... base/time/time.h:538: } On 2016/05/20 19:44:56, mmenke wrote: > Is the plan to replace calls to the old methods with calls to these? Yes, I would like to do so. https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h#newco... base/time/time.h:579: static Time FromExploded(bool is_local, const Exploded& exploded); On 2016/05/20 19:44:56, mmenke wrote: > If we're going to keep the old methods around (Permanently? Temporarily?), we > should probably still get rid of this method, and make the two old public > functions wrap the new FromExploded function. > > Also, since the new methods have WARN_UNUSED_RESULT in the new methods, you'll > need to use ignore_result on the calls in the two old methods to the new > FromExploded method. Done. https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h#newco... base/time/time.h:586: static bool FromExploded(bool is_local, const Exploded& exploded, Time& time); On 2016/05/20 19:44:56, mmenke wrote: > WARN_UNUSED_RESULT Done. https://codereview.chromium.org/1988663002/diff/160001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1988663002/diff/160001/base/time/time_mac.cc#... base/time/time_mac.cc:188: base::Time t_time = On 2016/05/20 19:44:56, mmenke wrote: > nit: t_time seems like a weird variable name - it's not clear what the "t_" > means. I'd suggest just calling it "time". Done. I already have a "time" in the function arguments. "out_time" will be a great name, I think. https://codereview.chromium.org/1988663002/diff/160001/base/time/time_mac.cc#... base/time/time_mac.cc:195: // |utc_to_exploded| time. On 2016/05/20 19:44:56, mmenke wrote: > nit: Comment can be reformatted - first line of text is unusually short. Done. https://codereview.chromium.org/1988663002/diff/160001/base/time/time_posix.cc File base/time/time_posix.cc (right): https://codereview.chromium.org/1988663002/diff/160001/base/time/time_posix.c... base/time/time_posix.cc:305: kWindowsEpochDeltaMicroseconds); On 2016/05/20 19:44:57, mmenke wrote: > Again, suggest just calling this |time|, unless there's another variable with > that name I'm missing. In which case, could call it something like out, result, > out_time, etc. Done. https://codereview.chromium.org/1988663002/diff/160001/base/time/time_posix.c... base/time/time_posix.cc:318: return to_exploded != exploded ? Time(0) : t_time; On 2016/05/20 19:44:57, mmenke wrote: > nit: Fix indent. You can automatically format everything your CL touches with > "git cl format". Done. https://codereview.chromium.org/1988663002/diff/160001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/1988663002/diff/160001/base/time/time_win.cc#... base/time/time_win.cc:288: success = TzSpecificLocalTimeToSystemTime(NULL, &st, &utc_st) && On 2016/05/20 19:44:57, mmenke wrote: > nit: nullptr is now preferred over NULL. Done. https://codereview.chromium.org/1988663002/diff/160001/base/time/time_win.cc#... base/time/time_win.cc:295: NOTREACHED() << "Unable to convert time"; On 2016/05/20 19:44:57, mmenke wrote: > Looks like this NOTREACHED is incorrect (In the old code, too). We just never > tested it with invalid times, and it's rare enough to see them, that we never > ran into the issue. Done.
I'm happy with this, just nits. https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newco... base/time/time.h:523: // into a Time class. Should add a TODO here: TODO(maksims): Get rid of these in favor of the methods below. Note that the TODO isn't a promise that you'll do it, just a pointer to who has context (I think it's reasonable for you to take it on yourself in a followup CL, and doesn't look too bad, just explaining how things work). https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newco... base/time/time.h:538: // FromExploded return true on success or false on failure. For example, Think your second sentence shouldn't refer to the old methods. Instead, should just replace the second sentence with "Returns false on failure. For example, ..." https://codereview.chromium.org/1988663002/diff/220001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time_mac.cc#... base/time/time_mac.cc:206: return (*time).is_null() ? false : true; Time(0) is actually a legal time, so suggest instead (Starting from line 202): if (to_exploded != exploded) { *time = Time(0); return false; } *time = converted_time; return true; https://codereview.chromium.org/1988663002/diff/220001/base/time/time_posix.cc File base/time/time_posix.cc (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time_posix.c... base/time/time_posix.cc:320: return (*time).is_null() ? false : true; See comment in mac file. https://codereview.chromium.org/1988663002/diff/220001/base/time/time_unittes... File base/time/time_unittest.cc (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time_unittes... base/time/time_unittest.cc:54: void PrintTo(const DateTestData& test_data, std::ostream* os) { I don't think this is used? https://codereview.chromium.org/1988663002/diff/220001/base/time/time_unittes... base/time/time_unittest.cc:90: testing::ValuesIn(kDateTestData)); Rather than use "INSTANTIATE_TEST_CASE_P", may just want to use a single TEST with a range-based for loop, like: TEST(TimeTestOutOfBounds) { const struct DateTestData =.... for (const auto& test : kDateTestData) { ... } } It's my experience that it's simpler to read and debug tests without TEST_P, when there's less macro magic going on.
https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newco... base/time/time.h:428: bool operator!=(const Exploded& rhs); Put a blank line before this, because it’s not related to HasValidValues(). Specify that this doesn’t actually consider day_of_week, unless you want it to, in which case you should update the implementation.
https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newco... base/time/time.h:428: bool operator!=(const Exploded& rhs); On 2016/05/24 18:27:27, Mark Mentovai wrote: > Put a blank line before this, because it’s not related to HasValidValues(). > > Specify that this doesn’t actually consider day_of_week, unless you want it to, > in which case you should update the implementation. I'm not sure we can check day of week...So as I believe someone suggested earlier, this probably should not be operator !=. Maybe instead make a private comparison function in base::Time, so people don't run into that weirdness, and call it something weird, like ExplodedMostlyEquals? If Mark disagrees, I defer to him on this.
https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newco... base/time/time.h:428: bool operator!=(const Exploded& rhs); mmenke wrote: > On 2016/05/24 18:27:27, Mark Mentovai wrote: > > Put a blank line before this, because it’s not related to HasValidValues(). > > > > Specify that this doesn’t actually consider day_of_week, unless you want it > to, > > in which case you should update the implementation. > > I'm not sure we can check day of week...So as I believe someone suggested > earlier, this probably should not be operator !=. > > Maybe instead make a private comparison function in base::Time, so people don't > run into that weirdness, and call it something weird, like ExplodedMostlyEquals? > If Mark disagrees, I defer to him on this. Maybe it is better that we rename it, then.
Patchset #6 (id:240001) has been deleted
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:280001) has been deleted
Description was changed from ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If !=, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 ========== to ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If times are not equal, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 ==========
Patchset #6 (id:300001) has been deleted
https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newco... base/time/time.h:428: bool operator!=(const Exploded& rhs); On 2016/05/24 18:34:13, Mark Mentovai wrote: > mmenke wrote: > > On 2016/05/24 18:27:27, Mark Mentovai wrote: > > > Put a blank line before this, because it’s not related to HasValidValues(). > > > > > > Specify that this doesn’t actually consider day_of_week, unless you want it > > to, > > > in which case you should update the implementation. > > > > I'm not sure we can check day of week...So as I believe someone suggested > > earlier, this probably should not be operator !=. > > > > Maybe instead make a private comparison function in base::Time, so people > don't > > run into that weirdness, and call it something weird, like > ExplodedMostlyEquals? > > If Mark disagrees, I defer to him on this. > > Maybe it is better that we rename it, then. Done. https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newco... base/time/time.h:523: // into a Time class. On 2016/05/24 16:08:33, mmenke wrote: > Should add a TODO here: > > TODO(maksims): Get rid of these in favor of the methods below. > > Note that the TODO isn't a promise that you'll do it, just a pointer to who has > context (I think it's reasonable for you to take it on yourself in a followup > CL, and doesn't look too bad, just explaining how things work). Done. https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newco... base/time/time.h:538: // FromExploded return true on success or false on failure. For example, On 2016/05/24 16:08:33, mmenke wrote: > Think your second sentence shouldn't refer to the old methods. > > Instead, should just replace the second sentence with "Returns false on failure. > For example, ..." Done. https://codereview.chromium.org/1988663002/diff/220001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time_mac.cc#... base/time/time_mac.cc:206: return (*time).is_null() ? false : true; On 2016/05/24 16:08:33, mmenke wrote: > Time(0) is actually a legal time, so suggest instead (Starting from line 202): > > if (to_exploded != exploded) { > *time = Time(0); > return false; > } > > *time = converted_time; > return true; Done. https://codereview.chromium.org/1988663002/diff/220001/base/time/time_posix.cc File base/time/time_posix.cc (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time_posix.c... base/time/time_posix.cc:320: return (*time).is_null() ? false : true; On 2016/05/24 16:08:33, mmenke wrote: > See comment in mac file. Done. https://codereview.chromium.org/1988663002/diff/220001/base/time/time_unittes... File base/time/time_unittest.cc (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time_unittes... base/time/time_unittest.cc:54: void PrintTo(const DateTestData& test_data, std::ostream* os) { On 2016/05/24 16:08:33, mmenke wrote: > I don't think this is used? it is automatically invoked by gtest, when using TestWithParam https://codereview.chromium.org/1988663002/diff/220001/base/time/time_unittes... base/time/time_unittest.cc:90: testing::ValuesIn(kDateTestData)); On 2016/05/24 16:08:33, mmenke wrote: > Rather than use "INSTANTIATE_TEST_CASE_P", may just want to use a single TEST > with a range-based for loop, like: > > TEST(TimeTestOutOfBounds) { > const struct DateTestData =.... > > for (const auto& test : kDateTestData) { > ... > } > } > > It's my experience that it's simpler to read and debug tests without TEST_P, > when there's less macro magic going on. Done.
https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h#newco... base/time/time.h:527: ignore_result(FromExploded(false, exploded, &time)); Centralize the “false”/“true”. This should be ignore_result(FromUTCExploded(exploded, &time)). Similar on line 532. Rationale: “false”/“true” require reading the definition to understand whether it’s “is_local” or “is_utc” or something else entirely. Since it’s a private implementation detail, it’s not worth adding an enum to clarify this at call sites, but since equivalent function names are available, you should use them. https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h#newco... base/time/time.h:539: // which results in 1st day of the next month. Nuke “which results in 1st day of the next month.” We’re not defining what the 31st day of a 30-day month means, we’re saying that it’s a failure. https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h#newco... base/time/time.h:606: static bool ExplodedMostlyEquals(const Exploded& lhs, const Exploded& rhs); Not sure why this isn’t a non-static MostlyEquals() defined as a member of Exploded. Is there a reason?
https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h#newco... base/time/time.h:606: static bool ExplodedMostlyEquals(const Exploded& lhs, const Exploded& rhs); On 2016/05/26 14:08:39, Mark Mentovai wrote: > Not sure why this isn’t a non-static MostlyEquals() defined as a member of > Exploded. Is there a reason? If we put it there, we'd either need to make it public, which seems not great, or make this class a friend of Exploded.
ptal https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h#newco... base/time/time.h:606: static bool ExplodedMostlyEquals(const Exploded& lhs, const Exploded& rhs); On 2016/05/26 14:45:08, mmenke wrote: > On 2016/05/26 14:08:39, Mark Mentovai wrote: > > Not sure why this isn’t a non-static MostlyEquals() defined as a member of > > Exploded. Is there a reason? > > If we put it there, we'd either need to make it public, which seems not great, > or make this class a friend of Exploded. Yes, exactly. Should I change it to be a friend then?
Since it’s private, it’s not the end of the world to leave it where it is.
LGTM
LGTM
The CQ bit was checked by maksim.sisov@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988663002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988663002/340001
Message was sent while issue was closed.
Description was changed from ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If times are not equal, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 ========== to ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If times are not equal, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If times are not equal, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 ========== to ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If times are not equal, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 Committed: https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382 Cr-Commit-Position: refs/heads/master@{#396638} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382 Cr-Commit-Position: refs/heads/master@{#396638}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:340001) has been created in https://codereview.chromium.org/2022913002/ by guidou@chromium.org. The reason for reverting is: Suspect of breking Linux Tests bot. https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2... I will reland if the revert does not fix the bot..
Message was sent while issue was closed.
On 2016/05/31 08:18:45, Guido Urdaneta wrote: > A revert of this CL (patchset #7 id:340001) has been created in > https://codereview.chromium.org/2022913002/ by mailto:guidou@chromium.org. > > The reason for reverting is: Suspect of breking Linux Tests bot. > https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2... > > I will reland if the revert does not fix the bot.. The revert fixed the bot, so please take a look at what could have caused the breakage before relanding. https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2... https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2...
Message was sent while issue was closed.
Description was changed from ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If times are not equal, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 Committed: https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382 Cr-Commit-Position: refs/heads/master@{#396638} ========== to ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If times are not equal, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 Committed: https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382 Cr-Commit-Position: refs/heads/master@{#396638} ==========
The CQ bit was checked by maksim.sisov@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988663002/340001
Message was sent while issue was closed.
Description was changed from ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If times are not equal, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 Committed: https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382 Cr-Commit-Position: refs/heads/master@{#396638} ========== to ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If times are not equal, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 Committed: https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382 Cr-Commit-Position: refs/heads/master@{#396638} ==========
Message was sent while issue was closed.
Committed patchset #7 (id:340001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If times are not equal, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 Committed: https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382 Cr-Commit-Position: refs/heads/master@{#396638} ========== to ========== Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If times are not equal, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 Committed: https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382 Committed: https://crrev.com/558f168054566ecbc611918093398add04dc13a4 Cr-Original-Commit-Position: refs/heads/master@{#396638} Cr-Commit-Position: refs/heads/master@{#399794} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/558f168054566ecbc611918093398add04dc13a4 Cr-Commit-Position: refs/heads/master@{#399794}
Message was sent while issue was closed.
This partially broke FTP directory listings on Linux (and perhaps Mac). http://crbug.com/641096 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
