[Tarantool-patches] [PATCH v3 2/9] lua: built-in module datetime
Safin Timur
tsafin at tarantool.org
Sun Aug 8 19:47:27 MSK 2021
On 08.08.2021 17:34, Vladislav Shpilevoy wrote:
> 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.
OMG, Shit, that was unfortunate collision.
Leonid is that the recommended way now forward? What should we do within
patchset which is on the flight at the moment?
>
>> 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?
Not, all fields, there are (at least) 3 important fields left untouched:
tm_isdst, tm_gmtoff, and tm_tmzone. I have to reset them every time this
function got called.
>
>> + 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.
Yup.
>
>> +{
>> + 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?
gettimeofday to at least to get microseconds precision for secs/nsec.
And time() + localtime() for timezone information. We do not actually
interested to see time from time() + localtime() - only offset information.
Please give me know if there is some obvious, simpler way to retrieve
nanoseconds and timezone in less number of libc calls?
>
>> + 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.
Yes, reentrant version of those functions should help, will update patch
soon. Fortunately we already have unit test
test/app-tap/gh-5632-6050-6259-gc-buf-reuse.test.lua which checks GC
efficiently here. Thanks for the hint in prior email!
>
>> +}
>> +
>> +char *
>> +datetime_ctime(const struct datetime *date)
>> +{
>> + time_t time = date->secs;
>> + return ctime(&time);
>> +}
The same as above.
>> +
>> +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.
Ok.
>
>> +#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.
Yes, will clean it up.
>
> I will return later with more comments. Need to go now.
>
Thanks,
Timur
More information about the Tarantool-patches
mailing list