From: Safin Timur via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Serge Petrenko <sergepetrenko@tarantool.org> Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Subject: Re: [Tarantool-patches] [PATCH v5 1/8] build: add Christian Hansen c-dt to the build Date: Wed, 18 Aug 2021 02:24:38 +0300 [thread overview] Message-ID: <613e625e-591b-6c74-dfb6-0ba5f5f0a787@tarantool.org> (raw) In-Reply-To: <eadcddc4-5e26-f8f3-b481-df5883a333b1@tarantool.org> 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. > >> --- >> .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. > > > <stripped> >> + >> +/* 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 > > <stripped> > >> 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
next prev parent reply other threads:[~2021-08-17 23:24 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-15 23:59 [Tarantool-patches] [PATCH v5 0/8] Initial datetime implementation Timur Safin via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 1/8] build: add Christian Hansen c-dt to the build Timur Safin via Tarantool-patches 2021-08-17 12:15 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:24 ` Safin Timur via Tarantool-patches [this message] 2021-08-18 8:56 ` Serge Petrenko via Tarantool-patches 2021-08-17 15:50 ` Vladimir Davydov via Tarantool-patches 2021-08-18 10:04 ` Safin Timur via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 2/8] lua: built-in module datetime Timur Safin via Tarantool-patches 2021-08-17 12:15 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:30 ` Safin Timur via Tarantool-patches 2021-08-18 8:56 ` Serge Petrenko via Tarantool-patches 2021-08-17 16:52 ` Vladimir Davydov via Tarantool-patches 2021-08-17 19:16 ` Vladimir Davydov via Tarantool-patches 2021-08-18 13:38 ` Safin Timur via Tarantool-patches 2021-08-18 10:03 ` Safin Timur via Tarantool-patches 2021-08-18 10:06 ` Safin Timur via Tarantool-patches 2021-08-18 11:45 ` Vladimir Davydov via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 3/8] lua, datetime: display datetime Timur Safin via Tarantool-patches 2021-08-17 12:15 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:32 ` Safin Timur via Tarantool-patches 2021-08-17 17:06 ` Vladimir Davydov via Tarantool-patches 2021-08-18 14:10 ` Safin Timur via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 4/8] box, datetime: messagepack support for datetime Timur Safin via Tarantool-patches 2021-08-16 0:20 ` Safin Timur via Tarantool-patches 2021-08-17 12:15 ` Serge Petrenko via Tarantool-patches 2021-08-17 12:16 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:42 ` Safin Timur via Tarantool-patches 2021-08-18 9:01 ` Serge Petrenko via Tarantool-patches 2021-08-17 18:36 ` Vladimir Davydov via Tarantool-patches 2021-08-18 14:27 ` Safin Timur via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 5/8] box, datetime: datetime comparison for indices Timur Safin via Tarantool-patches 2021-08-17 12:16 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:43 ` Safin Timur via Tarantool-patches 2021-08-18 9:03 ` Serge Petrenko via Tarantool-patches 2021-08-17 19:05 ` Vladimir Davydov via Tarantool-patches 2021-08-18 17:18 ` Safin Timur via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 6/8] lua, datetime: time intervals support Timur Safin via Tarantool-patches 2021-08-17 12:16 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:44 ` Safin Timur via Tarantool-patches 2021-08-17 18:52 ` Vladimir Davydov via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 7/8] datetime: perf test for datetime parser Timur Safin via Tarantool-patches 2021-08-17 19:13 ` Vladimir Davydov via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 8/8] datetime: changelog for datetime module Timur Safin via Tarantool-patches 2021-08-17 12:16 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:44 ` Safin Timur via Tarantool-patches 2021-08-18 9:04 ` Serge Petrenko via Tarantool-patches 2021-08-17 12:15 ` [Tarantool-patches] [PATCH v5 0/8] Initial datetime implementation Serge Petrenko via Tarantool-patches [not found] ` <20210818082222.mofgheciutpipelz@esperanza> 2021-08-18 8:25 ` Vladimir Davydov via Tarantool-patches 2021-08-18 13:24 ` Safin Timur via Tarantool-patches 2021-08-18 14:22 ` Vladimir Davydov via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=613e625e-591b-6c74-dfb6-0ba5f5f0a787@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=sergepetrenko@tarantool.org \ --cc=tsafin@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v5 1/8] build: add Christian Hansen c-dt to the build' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox