From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id A15736EC40; Wed, 18 Aug 2021 13:04:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A15736EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629281072; bh=eLLfUQsVb23FW3a//p9ORGuuHwebpLqXI4irOd2/1TQ=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=UBLMd51C0YD4mHk/k8oed/OTGU4NFr5yhGjYWIuvVr5Rh3wTPyn2Uls9LbI6SG5i1 RALXlK8VF0pd+iEMlmFQ7z4QZ9ZSzTpbWCkPyEUcVLIOYcK90lwW1EqkaTKGkclJ6T +lRSVhqfXK3ncfhMQ62j308pbXP3b9z0A/jtnNTs= Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 56D236EC40 for ; Wed, 18 Aug 2021 13:04:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 56D236EC40 Received: by smtp51.i.mail.ru with esmtpa (envelope-from ) id 1mGIQu-0002WP-RK; Wed, 18 Aug 2021 13:04:29 +0300 To: Vladimir Davydov , Timur Safin via Tarantool-patches Cc: v.shpilevoy@tarantool.org References: <25f9e4a85319d7b4d1fa4fe70d9e9fb30b43c78d.1629071531.git.tsafin@tarantool.org> <20210817155019.zsvbxiz7dvc4cpo4@esperanza> Message-ID: <8454d354-51b8-95bb-e28d-12e87933adfa@tarantool.org> Date: Wed, 18 Aug 2021 13:04:16 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <20210817155019.zsvbxiz7dvc4cpo4@esperanza> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD91BCCB18F2C129F87F36E61E9E4584E9D182A05F53808504020BCB572160810D920997316095ED6A937295FBB46C9C380814825BECF6C8155 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7956F10FFCC7409BAEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006379C6BD13E0D9523298638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D82215052DF79E79407F9961B4C28F62CD117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B2EE5AD8F952D28FBA471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FC915BD78E5328D2483AA81AA40904B5D9CF19DD082D7633A078D18283394535A93AA81AA40904B5D98AA50765F7900637AFB017D115533FEFD81D268191BDAD3D698AB9A7B718F8C4D1B931868CE1C5781A620F70A64A45A98AA50765F79006372E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89FB26E97DCB74E6252A91E23F1B6B78B78B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5C75BDBC2E3AC19C006B4B23A77C494E4F8DD3ECFEDB37B94D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA757E10A58996CBD514410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34505665BFD4C707058E15BB3E074A29C2CA6226EA2393B8F4431611E1C387007B2EDD9B348AF717131D7E09C32AA3244C51226619F22F54D4E2472E72D3B45BC155E75C8D0ED9F6EEFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojuRQ/H5n28to6EogSYq+B1w== X-Mailru-Sender: B5B6A6EBBD94DAD872610222F811036B0578E22FD5DF8D8B3C9916D590A63BA90F81FB139F4278C21EC9E4A2C82A33BC8C24925A86E657CE0C70AEE3C9A96FBAB3D7EE8ED63280BE112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 1/8] build: add Christian Hansen c-dt to the build X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Safin Timur via Tarantool-patches Reply-To: Safin Timur Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thanks for your feedback, it's massive. Will take some time to process all changes... On 17.08.2021 18:50, Vladimir Davydov via Tarantool-patches wrote: > On Mon, Aug 16, 2021 at 02:59:35AM +0300, Timur Safin via > Tarantool-patches wrote: >> * Integrated chansen/c-dt parser as 3rd party module to the >> Tarantool cmake build process; >> * Points to tarantool/c-dt instead of original chansen/c-dt to >> have easier build integration, because there are additional >> commits which have integrated cmake support and have established >> symbols renaming facilities (similar to those we see in xxhash >> or icu). >> We have to be able to rename externally public symbols to avoid >> name clashes with 3rd party modules. We prefix c-dt symbols >> in the Tarantool build with `tnt_` prefix; > > I don't see this done in this patch. Looks like this is done by patch 2. Indeed, that was extracted for better observability of a patch. > >> * took special care that generated build artifacts not override >> in-source files, but use different build/ directory. >> >> * added datetime parsing unit tests, for literals - with and >> without trailing timezones; >> * also we check that strftime is reversible and produce consistent >> results after roundtrip from/to strings; >> * discovered the harder way that on *BSD/MacOSX `strftime()` format >> `%z` outputs local time-zone if passed `tm_gmtoff` is 0. >> This behaviour is different to that we observed on Linux, thus we >> might have different execution results. Made test to not use `%z` >> and only operate with normalized date time formats `%F` and `%T` >> >> Part of #5941 >> --- >> .gitmodules | 3 + >> CMakeLists.txt | 8 + >> cmake/BuildCDT.cmake | 8 + >> src/CMakeLists.txt | 3 +- >> test/unit/CMakeLists.txt | 3 +- >> test/unit/datetime.c | 223 ++++++++++++++++++++++++ >> test/unit/datetime.result | 358 ++++++++++++++++++++++++++++++++++++++ >> third_party/c-dt | 1 + >> 8 files changed, 605 insertions(+), 2 deletions(-) >> create mode 100644 cmake/BuildCDT.cmake >> create mode 100644 test/unit/datetime.c >> create mode 100644 test/unit/datetime.result >> create mode 160000 third_party/c-dt >> >> diff --git a/test/unit/datetime.c b/test/unit/datetime.c >> new file mode 100644 >> index 000000000..64c19dac4 >> --- /dev/null >> +++ b/test/unit/datetime.c >> @@ -0,0 +1,223 @@ >> +#include "dt.h" >> +#include >> +#include >> +#include >> +#include >> + >> +#include "unit.h" >> + >> +static const char sample[] = "2012-12-24T15:30Z"; >> + >> +#define S(s) {s, sizeof(s) - 1} >> +struct { >> + const char * sz; > > Extra space after '*'. > > We usually name a variable that stores a zero-terminated 's' or 'str', > never 'sz'. > >> +#define DIM(a) (sizeof(a) / sizeof(a[0])) > > There's 'lengthof' helper already defined for this. Thanks! Updated. > >> + >> +/* p5-time-moment/src/moment_parse.c: parse_string_lenient() */ > > I don't understand the purpose of this comment. That was referring to the original implementation of similar code in Christian Hansen time-moment module for perl5. Agreed that it has not much value right now, so removed it. > >> +static int >> +parse_datetime(const char *str, size_t len, int64_t *sp, int32_t *np, >> + int32_t *op) > > What's 'sp', 'np', and 'op'? Please refrain from using confusing > abbreviations. > >> +{ >> + size_t n; >> + dt_t dt; >> + char c; >> + int sod = 0, nanosecond = 0, offset = 0; >> + >> + n = dt_parse_iso_date(str, len, &dt); >> + if (!n) >> + return 1; >> + if (n == len) >> + goto exit; >> + >> + c = str[n++]; >> + if (!(c == 'T' || c == 't' || c == ' ')) >> + return 1; >> + >> + str += n; >> + len -= n; >> + >> + n = dt_parse_iso_time(str, len, &sod, &nanosecond); >> + if (!n) >> + return 1; >> + if (n == len) >> + goto exit; >> + >> + if (str[n] == ' ') >> + n++; > > Bad indentation. Fixed. > >> + >> + str += n; >> + len -= n; >> + >> + n = dt_parse_iso_zone_lenient(str, len, &offset); >> + if (!n || n != len) >> + return 1; >> + >> +exit: >> + *sp = ((int64_t)dt_rdn(dt) - 719163) * 86400 + sod - offset * 60; > > Please define and use appropriate constants to make the code easier for > understanding: DAYS_PER_YEAR, MINUTES_PER_HOUR, etc. Changed to use DT_EPOCH_1970_OFFSET and SECS_PER_DAY. > >> + *np = nanosecond; >> + *op = offset; >> + >> + return 0; >> +} >> + >> +/* avoid introducing external datetime.h dependency - >> + just copy paste it for today >> +*/ > > Bad comment formatting. This comment gone. Thanks! > >> +#define SECS_PER_DAY 86400 >> +#define DT_EPOCH_1970_OFFSET 719163 >> + >> +struct datetime { >> + double secs; >> + int32_t nsec; >> + int32_t offset; >> +}; > > I see that this struct as well as some functions in this module are > defined in src/lib/core/datetime in patch 2, and then you remove struct > datetime from this test in patch 3, but leave the helper functions > datetime_to_tm, parse_datetime. > > This makes me think that: > - You should squash patches 1-3. > - Both datetime and datetime manipulating functions (including > parse_datetime) should be defined only in src/lib/core/datetime > while test/unit should depend on src/lib/core/datetime. Thats's quite reasonable suggestion (and was my original intention), but for simplicity of review I tried split these huge patches into smaller ones (look s not very successfully, and introduced some inter-dependencies between patches). I'll squash. > >> + >> +static int >> +local_rd(const struct datetime *dt) >> +{ >> + return (int)((int64_t)dt->secs / SECS_PER_DAY) + > DT_EPOCH_1970_OFFSET; >> +} >> + >> +static int >> +local_dt(const struct datetime *dt) >> +{ >> + return dt_from_rdn(local_rd(dt)); >> +} >> + >> +struct tm * >> +datetime_to_tm(struct datetime *dt) >> +{ >> + static struct tm tm; >> + >> + memset(&tm, 0, sizeof(tm)); >> + dt_to_struct_tm(local_dt(dt), &tm); >> + >> + int seconds_of_day = (int64_t)dt->secs % 86400; >> + tm.tm_hour = (seconds_of_day / 3600) % 24; >> + tm.tm_min = (seconds_of_day / 60) % 60; >> + tm.tm_sec = seconds_of_day % 60; >> + >> + return &tm; >> +} >> + >> +static void datetime_test(void) >> +{ >> + size_t index; >> + int64_t secs_expected; >> + int32_t nanosecs; >> + int32_t ofs; > > offset? Yup, renamed. > >> + >> + plan(355); >> + parse_datetime(sample, sizeof(sample) - 1, >> + &secs_expected, &nanosecs, &ofs); >> + >> + for (index = 0; index < DIM(tests); index++) { >> + int64_t secs; >> + int rc = parse_datetime(tests[index].sz, tests[index].len, >> + &secs, &nanosecs, &ofs); >> + is(rc, 0, "correct parse_datetime return value for '%s'", >> + tests[index].sz); >> + is(secs, secs_expected, "correct parse_datetime output " >> + "seconds for '%s", tests[index].sz); >> + >> + /* check that stringized literal produces the same date */ >> + /* time fields */ >> + static char buff[40]; >> + struct datetime dt = {secs, nanosecs, ofs}; >> + /* datetime_to_tm returns time in GMT zone */ >> + struct tm * p_tm = datetime_to_tm(&dt); > > Extra space after '*'. Removed > >> + size_t len = strftime(buff, sizeof buff, "%F %T", p_tm); >> + ok(len > 0, "strftime"); >> + int64_t parsed_secs; >> + int32_t parsed_nsecs, parsed_ofs; >> + rc = parse_datetime(buff, len, &parsed_secs, > &parsed_nsecs, &parsed_ofs); >> + is(rc, 0, "correct parse_datetime return value for '%s'", > buff); >> + is(secs, parsed_secs, >> + "reversible seconds via strftime for '%s", buff); >> + } >> +} >> + >> +int >> +main(void) >> +{ >> + plan(1); >> + datetime_test(); >> + >> + return check_plan(); >> +} Here are incremental changes so far: -------------------------------------------------------- diff --git a/test/unit/datetime.c b/test/unit/datetime.c index 18b24927e..bfc752c59 100644 --- a/test/unit/datetime.c +++ b/test/unit/datetime.c @@ -9,12 +9,13 @@ #include "mp_datetime.h" #include "msgpuck.h" #include "mp_extension_types.h" +#include "trivia/util.h" static const char sample[] = "2012-12-24T15:30Z"; #define S(s) {s, sizeof(s) - 1} struct { - const char * sz; + const char *str; size_t len; } tests[] = { S("2012-12-24 15:30Z"), @@ -91,17 +92,14 @@ struct { }; #undef S -#define DIM(a) (sizeof(a) / sizeof(a[0])) - -/* p5-time-moment/src/moment_parse.c: parse_string_lenient() */ static int -parse_datetime(const char *str, size_t len, int64_t *sp, int32_t *np, - int32_t *op) +parse_datetime(const char *str, size_t len, int64_t *secs_p, + int32_t *nanosecs_p, int32_t *offset_p) { size_t n; dt_t dt; char c; - int sod = 0, nanosecond = 0, offset = 0; + int sec_of_day = 0, nanosecond = 0, offset = 0; n = dt_parse_iso_date(str, len, &dt); if (!n) @@ -116,14 +114,14 @@ parse_datetime(const char *str, size_t len, int64_t *sp, int32_t *np, str += n; len -= n; - n = dt_parse_iso_time(str, len, &sod, &nanosecond); + n = dt_parse_iso_time(str, len, &sec_of_day, &nanosecond); if (!n) return 1; if (n == len) goto exit; if (str[n] == ' ') - n++; + n++; str += n; len -= n; @@ -133,9 +131,10 @@ parse_datetime(const char *str, size_t len, int64_t *sp, int32_t *np, return 1; exit: - *sp = ((int64_t)dt_rdn(dt) - 719163) * 86400 + sod - offset * 60; - *np = nanosecond; - *op = offset; + *secs_p = ((int64_t)dt_rdn(dt) - DT_EPOCH_1970_OFFSET) * SECS_PER_DAY + + sec_of_day - offset * 60; + *nanosecs_p = nanosecond; + *offset_p = offset; return 0; } @@ -173,29 +172,30 @@ static void datetime_test(void) size_t index; int64_t secs_expected; int32_t nanosecs; - int32_t ofs; + int32_t offset; plan(355); parse_datetime(sample, sizeof(sample) - 1, - &secs_expected, &nanosecs, &ofs); + &secs_expected, &nanosecs, &offset); - for (index = 0; index < DIM(tests); index++) { + for (index = 0; index < lengthof(tests); index++) { int64_t secs; - int rc = parse_datetime(tests[index].sz, tests[index].len, - &secs, &nanosecs, &ofs); + int rc = parse_datetime(tests[index].str, tests[index].len, + &secs, &nanosecs, &offset); is(rc, 0, "correct parse_datetime return value for '%s'", - tests[index].sz); + tests[index].str); is(secs, secs_expected, "correct parse_datetime output " - "seconds for '%s", tests[index].sz); + "seconds for '%s", + tests[index].str); /* * check that stringized literal produces the same date * time fields */ static char buff[40]; - struct datetime dt = {secs, nanosecs, ofs}; + struct datetime dt = {secs, nanosecs, offset}; /* datetime_to_tm returns time in GMT zone */ - struct tm * p_tm = datetime_to_tm(&dt); + struct tm *p_tm = datetime_to_tm(&dt); size_t len = strftime(buff, sizeof buff, "%F %T", p_tm); ok(len > 0, "strftime"); int64_t parsed_secs; @@ -237,7 +237,7 @@ tostring_datetime_test(void) size_t index; plan(15); - for (index = 0; index < DIM(tests); index++) { + for (index = 0; index < lengthof(tests); index++) { struct datetime date = { tests[index].secs, tests[index].nsec, @@ -282,7 +282,7 @@ mp_datetime_test() size_t index; plan(85); - for (index = 0; index < DIM(tests); index++) { + for (index = 0; index < lengthof(tests); index++) { struct datetime date = { tests[index].secs, tests[index].nsec, -------------------------------------------------------- Thanks, Timur