[Tarantool-patches] [PATCH v5 1/8] build: add Christian Hansen c-dt to the build
Safin Timur
tsafin at tarantool.org
Wed Aug 18 02:24:38 MSK 2021
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
More information about the Tarantool-patches
mailing list