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 DC2326EC40; Wed, 18 Aug 2021 11:56:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DC2326EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629277009; bh=UobB9Qrm8RYTrbiQg2vGOublmHnOE5FiyeNmCM8vC30=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=gc3MbdQRW/bOGp96gEfeBr+6YOF3CKwQAU5l45CX2vMx1HoFnJdqP4hGPY3ODKbYZ 1cf4ThJb3iVA3U8+bZHoNjebQ9bzuhpBujZ1OomIxLQKUnczK2ydSJ+Eo/FXKAGp9S U6Q0FBbWUBmW+yd8u2c5DaqG1ou9a7KZXXZHMPE8= Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 4431A6EC40 for ; Wed, 18 Aug 2021 11:56:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4431A6EC40 Received: by smtp63.i.mail.ru with esmtpa (envelope-from ) id 1mGHNJ-0002j5-PW; Wed, 18 Aug 2021 11:56:42 +0300 To: Safin Timur References: <25f9e4a85319d7b4d1fa4fe70d9e9fb30b43c78d.1629071531.git.tsafin@tarantool.org> <613e625e-591b-6c74-dfb6-0ba5f5f0a787@tarantool.org> Message-ID: Date: Wed, 18 Aug 2021 11:56:41 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <613e625e-591b-6c74-dfb6-0ba5f5f0a787@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: ru X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD910164DC12A5633065676A9727AC27C74182A05F5380850409521C3298E9D646BB6F9A088744569A210FE2D4F245E51D6EDCB29A289EABCFB X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE79961E86438F5BDAEEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637E0B09B181166DBFC8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8F82580AB44DF2004793139ADBC69AB85117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EED76C6ED7039589DE4D0DA9BD313A0613D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE34CB6874B0BCFF0B803F1AB874ED89028C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F790063788B3B24285A3CD0EEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A556888888B3359E6E64B9617AC245B75D34383242880533E1D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA757E10A58996CBD514410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D340A59E724FC7897F74D8FDFC71D81EC76D272AE1A199DFFEF8020136B3984253E0EAE9D691C36A4701D7E09C32AA3244C46C8369E605E11FB8DAA4F1ABDBF3248E646F07CC2D4F3D883B48618A63566E0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojuRQ/H5n28tqTGXRimgAxKA== X-Mailru-Sender: 583F1D7ACE8F49BD31DE23046B3A846091D45FDF3C436872FFDFEE22CD711B8AA74AC2315E3D4B056BB2E709EA627F343C7DDD459B58856F0E45BC603594F5A135B915D4279FF0579437F6177E88F7363CDA0F3B3F5B9367 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: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 18.08.2021 02:24, Safin Timur пишет: > Thanks, for quick review, please see my notes below... > > On 17.08.2021 15:15, Serge Petrenko wrote: >> >> Hi! Thanks for the patch! >> >> Please, find a couple of style comments below. >> >> Also I think you may squash all the commits regarding c-dt cmake >> integration >> into one. And push all the commits from your branch to tarantool/c-dt >> master. >> It's not good that they live on a separate branch > > I wanted to keep original Christian Hansen' master intact, that's why > I've pushed all our changes to the separate branch.  Agreed though > they make no much sense separately, and squashed them together. Also > renamed branch `cmake` to `tarantool-master` and has made it's default > in repository. Yep, that's fine. > >> >>> --- >>>   .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/.gitmodules b/.gitmodules >>> index f2f91ee72..aa3fbae4e 100644 >>> --- a/.gitmodules >>> +++ b/.gitmodules >>> @@ -43,3 +43,6 @@ >>>   [submodule "third_party/xxHash"] >>>       path = third_party/xxHash >>>       url =https://github.com/tarantool/xxHash >>> +[submodule "third_party/c-dt"] >>> +    path = third_party/c-dt >>> +    url =https://github.com/tarantool/c-dt.git >>> diff --git a/CMakeLists.txt b/CMakeLists.txt >>> index e25b81eac..53c86f2a5 100644 >>> --- a/CMakeLists.txt >>> +++ b/CMakeLists.txt >>> @@ -571,6 +571,14 @@ endif() >>>   # zstd >>>   # >>> +# >>> +# Chritian Hanson c-dt > > BTW, I've spelled his name wrongly. Fixed and push-forced updated branch. > >>> +# >>> + >>> +include(BuildCDT) >>> +libccdt_build() >>> +add_dependencies(build_bundled_libs cdt) >>> + >>>   # >>>   # Third-Party misc >>>   # >>> diff --git a/cmake/BuildCDT.cmake b/cmake/BuildCDT.cmake >>> new file mode 100644 >>> index 000000000..343fb1b99 >>> --- /dev/null >>> +++ b/cmake/BuildCDT.cmake >>> @@ -0,0 +1,8 @@ >>> +macro(libccdt_build) >>> +    set(LIBCDT_INCLUDE_DIRS ${PROJECT_SOURCE_DIR}/third_party/c-dt/) >>> +    set(LIBCDT_LIBRARIES cdt) >>> + >>> +    file(MAKE_DIRECTORY >>> ${CMAKE_CURRENT_BINARY_DIR}/third_party/c-dt/build/) >>> +    add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/c-dt >>> + ${CMAKE_CURRENT_BINARY_DIR}/third_party/c-dt/build/) >>> +endmacro() >>> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt >>> index adb03b3f4..97b0cb326 100644 >>> --- a/src/CMakeLists.txt >>> +++ b/src/CMakeLists.txt >>> @@ -193,7 +193,8 @@ target_link_libraries(server core coll >>> http_parser bit uri uuid swim swim_udp >>>   # Rule of thumb: if exporting a symbol from a static library, list >>> the >>>   # library here. >>>   set (reexport_libraries server core misc bitset csv swim swim_udp >>> swim_ev >>> -     shutdown ${LUAJIT_LIBRARIES} ${MSGPUCK_LIBRARIES} >>> ${ICU_LIBRARIES} ${CURL_LIBRARIES} ${XXHASH_LIBRARIES}) >>> +     shutdown ${LUAJIT_LIBRARIES} ${MSGPUCK_LIBRARIES} >>> ${ICU_LIBRARIES} >>> +     ${CURL_LIBRARIES} ${XXHASH_LIBRARIES} ${LIBCDT_LIBRARIES}) >>>   set (common_libraries >>>       ${reexport_libraries} >>> diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt >>> index 5bb7cd6e7..31b183a8f 100644 >>> --- a/test/unit/CMakeLists.txt >>> +++ b/test/unit/CMakeLists.txt >>> @@ -56,7 +56,8 @@ add_executable(uuid.test uuid.c core_test_utils.c) >>>   target_link_libraries(uuid.test uuid unit) >>>   add_executable(random.test random.c core_test_utils.c) >>>   target_link_libraries(random.test core unit) >>> - >>> +add_executable(datetime.test datetime.c) >>> +target_link_libraries(datetime.test cdt unit) >>>   add_executable(bps_tree.test bps_tree.cc) >>>   target_link_libraries(bps_tree.test small misc) >>>   add_executable(bps_tree_iterator.test bps_tree_iterator.cc) >>> 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 >> >> I see that you only test datetime parsing in this test. >> Not the datetime module itself. >> Maybe worth renaming the test to datetime_parse, or c-dt, >> or any other name you find suitable? >> >> P.S. never mind, I see more tests are added to this file later on. > > Yes. > >> >> >> >>> + >>> +/* avoid introducing external datetime.h dependency - >>> +   just copy paste it for today >>> +*/ >> >> Please, fix comment formatting: >> >> /* Something you have to say. */ >> >> /* >>   * Something you have to say >>   * spanning a couple of lines. >>   */ >> > > I've deleted this block of code with comment, after I've introduced > datetime.h. So I'd rather delete this comment. > > >>> +#define SECS_PER_DAY      86400 >>> +#define DT_EPOCH_1970_OFFSET 719163 >>> + >>> +struct datetime { >>> +    double secs; >>> +    int32_t nsec; >>> +    int32_t offset; >>> +}; >>> + >>> +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; >>> + >>> +    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); >> >> >> Please, fix argument alignment here. > > Fixed. > >> >> >>> +        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 */ >> >> >> Same as above, please fix comment formatting. >> > > Ditto. > >> >>> +        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); >>> +        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(); >>> +} >>> diff --git a/test/unit/datetime.result b/test/unit/datetime.result >>> new file mode 100644 >>> index 000000000..33997d9df >>> --- /dev/null >>> +++ b/test/unit/datetime.result >> >> >> >>> diff --git a/third_party/c-dt b/third_party/c-dt >>> new file mode 160000 >>> index 000000000..5b1398ca8 >>> --- /dev/null >>> +++ b/third_party/c-dt >>> @@ -0,0 +1 @@ >>> +Subproject commit 5b1398ca8f53513985670a2e832b5f6e253addf7 >>> -- >>> Serge Petrenko >> > > Here is incremental update for this patch so far (applied to branch, > but not yet pushed - to accumulate all changes after Vova feedback): > -------------------------------------------------------------------- > diff --git a/CMakeLists.txt b/CMakeLists.txt > index 53c86f2a5..8037c30a7 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -572,7 +572,7 @@ endif() >  # > >  # > -# Chritian Hanson c-dt > +# Christian Hansen c-dt >  # > >  include(BuildCDT) > diff --git a/test/unit/datetime.c b/test/unit/datetime.c > index 64c19dac4..827930fc4 100644 > --- a/test/unit/datetime.c > +++ b/test/unit/datetime.c > @@ -136,16 +136,13 @@ exit: >         return 0; >  } > > -/* avoid introducing external datetime.h dependency - > -   just copy paste it for today > -*/ > -#define SECS_PER_DAY      86400 > +#define SECS_PER_DAY         86400 >  #define DT_EPOCH_1970_OFFSET 719163 > >  struct datetime { > -       double secs; > -       int32_t nsec; > -       int32_t offset; > +       double   secs; > +       uint32_t nsec; > +       int32_t  offset; >  }; > >  static int > @@ -190,14 +187,16 @@ static void datetime_test(void) >         for (index = 0; index < DIM(tests); index++) { >                 int64_t secs; >                 int rc = parse_datetime(tests[index].sz, > tests[index].len, > -                                               &secs, &nanosecs, &ofs); > +                                       &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 */ > +               /* > +                * 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 */ > diff --git a/third_party/c-dt b/third_party/c-dt > index 5b1398ca8..3cbbbc7f0 160000 > --- a/third_party/c-dt > +++ b/third_party/c-dt > @@ -1 +1 @@ > -Subproject commit 5b1398ca8f53513985670a2e832b5f6e253addf7 > +Subproject commit 3cbbbc7f032cfa67a8df9f81101403249825d7f3 > -------------------------------------------------------------------- > > Thanks, > Timur -- Serge Petrenko