Re: [Tarantool-patches] [PATCH v5 1/8] build: add Christian Hansen c-dt to the build

[Safin Timur via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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 <assert.h>
>> +#include <stdint.h>
>> +#include <string.h>
>> +#include <time.h>
>> +
>> +#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