[Tarantool-patches] [PATCH v5 1/8] build: add Christian Hansen c-dt to the build
Serge Petrenko
sergepetrenko at tarantool.org
Wed Aug 18 11:56:41 MSK 2021
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.
>
>>
>>
>> <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
--
Serge Petrenko
More information about the Tarantool-patches
mailing list