|
|
Created:
7 years, 2 months ago by Edmund.Grimley.Evans Modified:
7 years, 2 months ago CC:
dynamorio-devs_googlegroups.com Visibility:
Public. |
DescriptionCommit log for first patchset:
---------------
i#2121: Reimplement convert_millis_to_date and convert_date_to_millis.
Also add a unit test to check that these two functions are consistent.
Fixes #2121
---------------
Patch Set 1 #
Total comments: 4
Patch Set 2 : comments #
Total comments: 4
Patch Set 3 : Committed #MessagesTotal messages: 11
Sign in to reply to this message.
The original code is straightforward and readable. Let's make the new code have those properties as well. Please explain the constants and the 1200. https://codereview.appspot.com/311540043/diff/1/core/utils.c File core/utils.c (right): https://codereview.appspot.com/311540043/diff/1/core/utils.c#newcode3505 core/utils.c:3505: time += DAYS_IN_400_YEARS + 366 - 31 - 29; ? Why add? Why 31 and 29? Jan and Feb? Please comment. https://codereview.appspot.com/311540043/diff/1/core/utils.c#newcode3506 core/utils.c:3506: /* Time is now number of days since Wed, 1 Mar 1200 (Gregorian). */ ? Why 1200? https://codereview.appspot.com/311540043/diff/1/core/utils.c#newcode3517 core/utils.c:3517: dr_time->day_of_week = (days + 3) % 7; /* Sunday is 0. */ ? Why +3? https://codereview.appspot.com/311540043/diff/1/core/utils.c#newcode3522 core/utils.c:3522: days -= k * 146097 / 4; The comment says one thing, but then we have other constants here with no explanation. Ditto below.
Sign in to reply to this message.
On 2017/01/16 17:04:36, bruening wrote: > The original code is straightforward and readable. Let's make the new code have > those properties as well. Please explain the constants and the 1200. I could add some comments, though I'm not sure how useful they would be. Usually I find that the only way to get this kind of code correct is to test it, exhaustively, which is possible because there is a small, finite set of possible inputs. If I carefully work out what a constant should be, nearly always I find I made a mistake somewhere, while trial and error gets me to the right value for a small constant quite quickly. Do you want explanations for the constants in convert_date_to_millis, or are you happy to accept a well-known (Wikipedia, etc.) formula as it is? Perhaps there's a well-known formula for the other direction that could be copied from somewhere with small changes...
Sign in to reply to this message.
On 2017/01/17 11:46:24, Edmund.Grimley.Evans wrote: > On 2017/01/16 17:04:36, bruening wrote: > > The original code is straightforward and readable. Let's make the new code > have > > those properties as well. Please explain the constants and the 1200. > > I could add some comments, though I'm not sure how useful they would be. Usually > I find that the only way to get this kind of code correct is to test it, > exhaustively, which is possible because there is a small, finite set of possible > inputs. If I carefully work out what a constant should be, nearly always I find > I made a mistake somewhere, while trial and error gets me to the right value for > a small constant quite quickly. Mostly it's all the little constants: where did they come from? For each, a comment explaining what formula produced it would help immensely. Think about some future developer: I just don't think they could understand and thus maintain this code. At a higher level as well, I don't understand the general approach: why go back to 1200? > Do you want explanations for the constants in convert_date_to_millis, or are you > happy to accept a well-known (Wikipedia, etc.) formula as it is? Perhaps there's > a well-known formula for the other direction that could be copied from somewhere > with small changes... Are you sure the code is correct? Where did "307" come from? Also, the "Converting Julian or Gregorian calendar date to Julian day number" formula computes days since March 1, 4801 BC, and I don't see a correction for March 1 vs Jan 1. You have to admit that the original code is easier to understand and reason about.
Sign in to reply to this message.
> At a higher level as well, I don't understand the general approach: why go back > to 1200? I've changed it to 1600, which is just as good. > Are you sure the code is correct? It would be exaggeration to say I'm "sure", but the unit test compares convert_date_to_millis with convert_millis_to_date, and I've compared convert_millis_to_date with gmtime on a 64-bit Linux system...
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#2121: Reimplement convert_millis_to_date and convert_date_to_millis. Also add a unit test to check that these two functions are consistent. Fixes #2121 Review-URL: https://codereview.appspot.com/311540043 ---------------
Sign in to reply to this message.
I am not a fan of cryptic formulas, but if you are sure this is correct then LGTM https://codereview.appspot.com/311540043/diff/20001/core/utils.c File core/utils.c (right): https://codereview.appspot.com/311540043/diff/20001/core/utils.c#newcode3492 core/utils.c:3492: uint days, year, month, k; Is there a more descriptive name than "k" https://codereview.appspot.com/311540043/diff/20001/core/utils.c#newcode3541 core/utils.c:3541: /* Determine month: divide by 30.6 = 153 / 5, with carefully adjusted rounding I assume 153 is 31+30+31+30+31 and 5 is for those 5 months: best to point that out explicitly. https://codereview.appspot.com/311540043/diff/20001/core/utils.c#newcode3549 core/utils.c:3549: days -= ((month + 4) * 153) / 5 - 122; This is not easy to understand. "determine experimentally" rather than from a formula does not inspire confidence. https://codereview.appspot.com/311540043/diff/20001/core/utils.c#newcode4331 core/utils.c:4331: if (res != millis || I hope this was tested against external values of truth and not just this internal consistency.
Sign in to reply to this message.
Committed as https://github.com/DynamoRIO/dynamorio/commit/f708cccf22ff7fbf14b5129d5cf16fd... Final commit log: --------------- i#2121: Reimplement convert_millis_to_date and convert_date_to_millis. Also add a unit test to check that these two functions are consistent. Fixes #2121 Review-URL: https://codereview.appspot.com/311540043 ---------------
Sign in to reply to this message.
On 2017/01/23 10:52:38, Edmund.Grimley.Evans wrote: > Committed as > https://github.com/DynamoRIO/dynamorio/commit/f708cccf22ff7fbf14b5129d5cf16fd... > > Final commit log: > --------------- > i#2121: Reimplement convert_millis_to_date and convert_date_to_millis. > > Also add a unit test to check that these two functions are consistent. > > Fixes #2121 > > Review-URL: https://codereview.appspot.com/311540043 > --------------- This broke the Windows build: https://ci.appveyor.com/project/derekbruening/dynamorio/build/1.0.26/job/lype... C:\projects\dynamorio\core\utils.c(4416) : error C2220: warning treated as error - no 'object' file generated C:\projects\dynamorio\core\utils.c(4416) : warning C4146: unary minus operator applied to unsigned type, result still unsigned C:\projects\dynamorio\core\utils.c(4421) : warning C4146: unary minus operator applied to unsigned type, result still unsigned C:\projects\dynamorio\core\utils.c(4434) : warning C4244: '=' : conversion from 'uint64' to 'uint', possible loss of data This is blocking me trying to get AppVeyor added so I will go ahead and fix this.
Sign in to reply to this message.
> This is blocking me trying to get AppVeyor added so I will go ahead and fix > this. Thanks. Those Windows warnings really are atrocious. Unary minus on an unsigned type is totally OK in my book; I do it all the time. Anyway, this might fix it, though perhaps the casts aren't needed. Perhaps changing the type of t is sufficient. index c193e35..ab4292b 100644 --- a/core/utils.c +++ b/core/utils.c @@ -4362,7 +4362,7 @@ unit_test_utils(void) char buf[128]; uint c, d; const char *s; - uint64 t; + int t; dr_time_t dr_time; # define DO_TEST(a, b, p, percent, fmt, result) \ @@ -4412,17 +4412,17 @@ unit_test_utils(void) /* Test each millisecond in first and last 100 seconds. */ for (t = 0; t < 100000; t++) { - test_date_conversion_millis(t); - test_date_conversion_millis(-t - 1); + test_date_conversion_millis((uint64)t); + test_date_conversion_millis((uint64)(-t - 1)); } /* Test each second in first and last day and a bit. */ for (t = 0; t < 100000; t++) { - test_date_conversion_millis(t * 1000); - test_date_conversion_millis(-(t * 1000) - 1); + test_date_conversion_millis((uint64)(t * 1000)); + test_date_conversion_millis((uint64)(-(t * 1000) - 1)); } /* Test each day from 1601 to 2148. */ for (t = 0; t < 200000; t++) - test_date_conversion_millis(t * 24 * 60 * 60 * 1000); + test_date_conversion_millis((uint64)t * 24 * 60 * 60 * 1000); /* Test the first of each month from 1601 to 99999. */ dr_time.day_of_week = 0; /* not checked */ dr_time.day = 1;
Sign in to reply to this message.
By the way, I've just noticed that the comment in front of that function, unit_test_utils, is now out of date.
Sign in to reply to this message.
|