Re: [Tarantool-patches] [PATCH v3 2/9] lua: built-in module datetime

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 7 comments below.

> diff --git a/src/exports.h b/src/exports.h
> index 5bb3e6a2b..fb4a8084f 100644
> --- a/src/exports.h
> +++ b/src/exports.h
> @@ -217,7 +217,32 @@ EXPORT(curl_url_set)
>  EXPORT(curl_version)
>  EXPORT(curl_version_info)
>  #endif /* EXPORT_LIBCURL_SYMBOLS */
> +EXPORT(datetime_asctime)
> +EXPORT(datetime_ctime)
> +EXPORT(datetime_now)
> +EXPORT(datetime_strftime)
>  EXPORT(decimal_unpack)
> +EXPORT(dt_dow)
> +EXPORT(dt_from_rdn)
> +EXPORT(dt_from_struct_tm)
> +EXPORT(dt_from_yd)
> +EXPORT(dt_from_ymd)
> +EXPORT(dt_from_yqd)
> +EXPORT(dt_from_ywd)
> +EXPORT(dt_parse_iso_date)
> +EXPORT(dt_parse_iso_time_basic)
> +EXPORT(dt_parse_iso_time_extended)
> +EXPORT(dt_parse_iso_time)
> +EXPORT(dt_parse_iso_zone_basic)
> +EXPORT(dt_parse_iso_zone_extended)
> +EXPORT(dt_parse_iso_zone_lenient)
> +EXPORT(dt_parse_iso_zone)
> +EXPORT(dt_rdn)
> +EXPORT(dt_to_struct_tm)
> +EXPORT(dt_to_yd)
> +EXPORT(dt_to_ymd)
> +EXPORT(dt_to_yqd)
> +EXPORT(dt_to_ywd)

1. In scope of https://github.com/tarantool/tarantool/pull/6265 the exported
symbols are heavily reworked. In particular, you should not export library
symbols as is, you need to wrap them into tt_cdt_* functions and export them
instead. Otherwise the users' modules, which link with their own version of
c-dt, will have conflicts.

>  EXPORT(error_ref)
>  EXPORT(error_set_prev)
>  EXPORT(error_unpack_unsafe)
> diff --git a/src/lib/core/datetime.c b/src/lib/core/datetime.c
> new file mode 100644
> index 000000000..e77b188b7
> --- /dev/null
> +++ b/src/lib/core/datetime.c
> @@ -0,0 +1,96 @@
> +/*
> + * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include <string.h>
> +#include <time.h>
> +
> +#include "trivia/util.h"
> +#include "datetime.h"
> +
> +static int
> +local_dt(int64_t secs)
> +{
> +	return dt_from_rdn((int)(secs / SECS_PER_DAY) + DT_EPOCH_1970_OFFSET);
> +}
> +
> +static struct tm *
> +datetime_to_tm(const struct datetime *date)
> +{
> +	static struct tm tm;
> +
> +	memset(&tm, 0, sizeof(tm));

2. Why do you need this memset if you overwrite all the
fields anyway?

> +	int64_t secs = date->secs;
> +	dt_to_struct_tm(local_dt(secs), &tm);
> +
> +	int seconds_of_day = date->secs % SECS_PER_DAY;
> +	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;
> +}
> +
> +void
> +datetime_now(struct datetime * now)

3. Please, do not use whitespace between * and the variable name.

> +{
> +	struct timeval tv;
> +	gettimeofday(&tv, NULL);
> +	now->secs = tv.tv_sec;
> +	now->nsec = tv.tv_usec * 1000;
> +
> +	time_t now_seconds;
> +	time(&now_seconds);
> +	struct tm tm;
> +	localtime_r(&now_seconds, &tm);

4. Wtf? 2 system calls just to get the current time? Can you use
just one? Why can't you use tv.tv_sec which you got above?

> +	now->offset = tm.tm_gmtoff / 60;
> +}
> +
> +char *
> +datetime_asctime(const struct datetime *date)
> +{
> +	struct tm *p_tm = datetime_to_tm(date);
> +	return asctime(p_tm);

5. You can't return a global buffer. You need to write the
result into a local one passed in the arguments. AFAIS, there
are functions with _r suffix which should help.

> +}
> +
> +char *
> +datetime_ctime(const struct datetime *date)
> +{
> +	time_t time = date->secs;
> +	return ctime(&time);
> +}
> +
> +size_t
> +datetime_strftime(const struct datetime *date, const char *fmt, char *buf,
> +		  uint32_t len)
> +{
> +	struct tm *p_tm = datetime_to_tm(date);
> +	return strftime(buf, len, fmt, p_tm);
> +}
> diff --git a/src/lib/core/datetime.h b/src/lib/core/datetime.h
> new file mode 100644
> index 000000000..6699b31d3
> --- /dev/null
> +++ b/src/lib/core/datetime.h
> @@ -0,0 +1,94 @@
> +#pragma once
> +/*
> + * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include "c-dt/dt.h"
> +
> +#if defined(__cplusplus)
> +extern "C"
> +{
> +#endif /* defined(__cplusplus) */
> +
> +#ifndef SECS_PER_DAY
> +#define SECS_PER_DAY	86400

6. You have a tab here after the name. Please, use whitespace.

> +#define DT_EPOCH_1970_OFFSET 719163
> +#endif
> diff --git a/src/lua/datetime.lua b/src/lua/datetime.lua
> new file mode 100644
> index 000000000..a562b6872
> --- /dev/null
> +++ b/src/lua/datetime.lua
> @@ -0,0 +1,638 @@
> +local ffi = require('ffi')
> +
> +ffi.cdef [[
> +
> +    /*
> +    `c-dt` library functions handles properly both positive and negative `dt`
> +    values, where `dt` is a number of dates since Rata Die date (0001-01-01).
> +
> +    For better compactness of our typical data in MessagePack stream we shift
> +    root of our time to the Unix Epoch date (1970-01-01), thus our 0 is
> +    actually dt = 719163.
> +
> +    So here is a simple formula how convert our epoch-based seconds to dt values
> +        dt = (secs / 86400) + 719163
> +    Where 719163 is an offset of Unix Epoch (1970-01-01) since Rata Die
> +    (0001-01-01) in dates.
> +
> +    */
> +    typedef int dt_t;
> +
> +    // dt_core.h
> +    typedef enum {
> +        DT_MON       = 1,
> +        DT_MONDAY    = 1,
> +        DT_TUE       = 2,
> +        DT_TUESDAY   = 2,
> +        DT_WED       = 3,
> +        DT_WEDNESDAY = 3,
> +        DT_THU       = 4,
> +        DT_THURSDAY  = 4,
> +        DT_FRI       = 5,
> +        DT_FRIDAY    = 5,
> +        DT_SAT       = 6,
> +        DT_SATURDAY  = 6,
> +        DT_SUN       = 7,
> +        DT_SUNDAY    = 7,
> +    } dt_dow_t;
> +
> +    dt_t     dt_from_rdn     (int n);
> +    dt_t     dt_from_yd      (int y, int d);
> +    dt_t     dt_from_ymd     (int y, int m, int d);
> +    dt_t     dt_from_yqd     (int y, int q, int d);
> +    dt_t     dt_from_ywd     (int y, int w, int d);
> +
> +    void     dt_to_yd        (dt_t dt, int *y, int *d);
> +    void     dt_to_ymd       (dt_t dt, int *y, int *m, int *d);
> +    void     dt_to_yqd       (dt_t dt, int *y, int *q, int *d);
> +    void     dt_to_ywd       (dt_t dt, int *y, int *w, int *d);
> +
> +    int      dt_rdn          (dt_t dt);
> +    dt_dow_t dt_dow          (dt_t dt);

7. dt_dow is not used in this file. The enum dt_dow_t either.
The same for dt_from_rdn, dt_from_yd, dt_from_yqd, dt_from_ywd,
dt_to_yd, dt_to_ymd, dt_to_yqd, dt_to_ywd. Please, check all the
functions.

I will return later with more comments. Need to go now.