Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
Cc: v.shpilevoy@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v5 1/8] build: add Christian Hansen c-dt to the build
Date: Tue, 17 Aug 2021 18:50:19 +0300	[thread overview]
Message-ID: <20210817155019.zsvbxiz7dvc4cpo4@esperanza> (raw)
In-Reply-To: <25f9e4a85319d7b4d1fa4fe70d9e9fb30b43c78d.1629071531.git.tsafin@tarantool.org>

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.

> * 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.

> +
> +/* p5-time-moment/src/moment_parse.c: parse_string_lenient() */

I don't understand the purpose of this comment.

> +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.

> +
> +	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.

> +	*np = nanosecond;
> +	*op = offset;
> +
> +	return 0;
> +}
> +
> +/* avoid introducing external datetime.h dependency -
> +   just copy paste it for today
> +*/

Bad comment formatting.

> +#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.

> +
> +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?

> +
> +	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 '*'.

> +		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();
> +}

  parent reply	other threads:[~2021-08-17 15:50 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
2021-08-18  8:56       ` Serge Petrenko via Tarantool-patches
2021-08-17 15:50   ` Vladimir Davydov via Tarantool-patches [this message]
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=20210817155019.zsvbxiz7dvc4cpo4@esperanza \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --cc=vdavydov@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