Re: [Tarantool-patches] [PATCH v5 3/8] lua, datetime: display datetime

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Mon, Aug 16, 2021 at 02:59:37AM +0300, Timur Safin via Tarantool-patches wrote:
> * introduced output routine for converting datetime
>   to their default output format.
> 
> * use this routine for tostring() in datetime.lua
> 
> Part of #5941
> ---
>  extra/exports                  |   1 +
>  src/lib/core/datetime.c        |  71 ++++++++++++++++++
>  src/lib/core/datetime.h        |   9 +++
>  src/lua/datetime.lua           |  35 +++++++++
>  test/app-tap/datetime.test.lua | 131 ++++++++++++++++++---------------
>  test/unit/CMakeLists.txt       |   2 +-
>  test/unit/datetime.c           |  61 +++++++++++----
>  7 files changed, 236 insertions(+), 74 deletions(-)
> 
> diff --git a/extra/exports b/extra/exports
> index 80eb92abd..2437e175c 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -152,6 +152,7 @@ datetime_asctime
>  datetime_ctime
>  datetime_now
>  datetime_strftime
> +datetime_to_string
>  decimal_unpack
>  decimal_from_string
>  decimal_unpack
> diff --git a/src/lib/core/datetime.c b/src/lib/core/datetime.c
> index c48295a6f..c24a0df82 100644
> --- a/src/lib/core/datetime.c
> +++ b/src/lib/core/datetime.c
> @@ -29,6 +29,8 @@
>   * SUCH DAMAGE.
>   */
>  
> +#include <assert.h>
> +#include <limits.h>
>  #include <string.h>
>  #include <time.h>
>  
> @@ -94,3 +96,72 @@ datetime_strftime(const struct datetime *date, const char *fmt, char *buf,
>  	struct tm *p_tm = datetime_to_tm(date);
>  	return strftime(buf, len, fmt, p_tm);
>  }
> +
> +#define SECS_EPOCH_1970_OFFSET ((int64_t)DT_EPOCH_1970_OFFSET * SECS_PER_DAY)
> +
> +/* NB! buf may be NULL, and we should handle it gracefully, returning

/**
 * ...

(according to our coding style)

> + * calculated length of output string
> + */
> +int
> +datetime_to_string(const struct datetime *date, char *buf, uint32_t len)
> +{
> +#define ADVANCE(sz)		\
> +	if (buf != NULL) { 	\
> +		buf += sz; 	\
> +		len -= sz; 	\
> +	}			\
> +	ret += sz;

Please use SNPRINT helper.

> +
> +	int offset = date->offset;
> +	/* for negative offsets around Epoch date we could get
> +	 * negative secs value, which should be attributed to
> +	 * 1969-12-31, not 1970-01-01, thus we first shift
> +	 * epoch to Rata Die then divide by seconds per day,
> +	 * not in reverse
> +	 */
> +	int64_t secs = (int64_t)date->secs + offset * 60 + SECS_EPOCH_1970_OFFSET;
> +	assert((secs / SECS_PER_DAY) <= INT_MAX);
> +	dt_t dt = dt_from_rdn(secs / SECS_PER_DAY);
> +
> +	int year, month, day, sec, ns, sign;
> +	dt_to_ymd(dt, &year, &month, &day);
> +
> +	int hour = (secs / 3600) % 24,
> +	    minute = (secs / 60) % 60;

Please define on a separate line:

  int hour = ...;
  int minute = ...;

> +	sec = secs % 60;

sec, secs, oh

Please use better names.

> +	ns = date->nsec;
> +
> +	int ret = 0;
> +	uint32_t sz = snprintf(buf, len, "%04d-%02d-%02dT%02d:%02d",
> +			       year, month, day, hour, minute);
> +	ADVANCE(sz);
> +	if (sec || ns) {
> +		sz = snprintf(buf, len, ":%02d", sec);
> +		ADVANCE(sz);
> +		if (ns) {
> +			if ((ns % 1000000) == 0)
> +				sz = snprintf(buf, len, ".%03d", ns / 1000000);
> +			else if ((ns % 1000) == 0)
> +				sz = snprintf(buf, len, ".%06d", ns / 1000);
> +			else
> +				sz = snprintf(buf, len, ".%09d", ns);
> +			ADVANCE(sz);
> +		}
> +	}
> +	if (offset == 0) {
> +		sz = snprintf(buf, len, "Z");
> +		ADVANCE(sz);
> +	}
> +	else {

Bad formatting. Should be '} else {'.

> +		if (offset < 0)
> +			sign = '-', offset = -offset;

Please don't abuse ','.

> +		else
> +			sign = '+';
> +
> +		sz = snprintf(buf, len, "%c%02d:%02d", sign, offset / 60, offset % 60);
> +		ADVANCE(sz);
> +	}
> +	return ret;
> +}
> +#undef ADVANCE
> +
> diff --git a/src/lib/core/datetime.h b/src/lib/core/datetime.h
> index 1a8d7e34f..964e76fcc 100644
> --- a/src/lib/core/datetime.h
> +++ b/src/lib/core/datetime.h
> @@ -70,6 +70,15 @@ struct datetime_interval {
>  	int32_t nsec;
>  };
>  
> +/**
> + * Convert datetime to string using default format
> + * @param date source datetime value
> + * @param buf output character buffer
> + * @param len size ofoutput buffer
> + */
> +int
> +datetime_to_string(const struct datetime *date, char *buf, uint32_t len);
> +

This should be an snprint-like function: buf and len should go first.
This would allow us to use it in conjunction with SNPRINT.

>  /**
>   * Convert datetime to string using default asctime format
>   * "Sun Sep 16 01:03:52 1973\n\0"
> diff --git a/src/lua/datetime.lua b/src/lua/datetime.lua
> index ce579828f..4d946f194 100644
> --- a/src/lua/datetime.lua
> +++ b/src/lua/datetime.lua
> @@ -249,6 +249,37 @@ local function datetime_new(obj)
>      return datetime_new_dt(dt, secs, frac, offset)
>  end
>  
> +local function datetime_tostring(o)

Please add a comment to this function with example output.

> +    if ffi.typeof(o) == datetime_t then
> +        local sz = 48
> +        local buff = ffi.new('char[?]', sz)
> +        local len = builtin.datetime_to_string(o, buff, sz)
> +        assert(len < sz)
> +        return ffi.string(buff)
> +    elseif ffi.typeof(o) == interval_t then

Please define separate functions for interval and datetime.

> +        local ts = o.timestamp
> +        local sign = '+'
> +
> +        if ts < 0 then
> +            ts = -ts
> +            sign = '-'
> +        end
> +
> +        if ts < 60 then
> +            return ('%s%s secs'):format(sign, ts)
> +        elseif ts < 60 * 60 then
> +            return ('%+d minutes, %s seconds'):format(o.minutes, ts % 60)
> +        elseif ts < 24 * 60 * 60 then
> +            return ('%+d hours, %d minutes, %s seconds'):format(
> +                    o.hours, o.minutes % 60, ts % 60)
> +        else
> +            return ('%+d days, %d hours, %d minutes, %s seconds'):format(
> +                    o.days, o.hours % 24, o.minutes % 60, ts % 60)
> +        end
> +    end
> +end
> +
> +
>  --[[
>      Basic      Extended
>      20121224   2012-12-24   Calendar date   (ISO 8601)
> @@ -457,6 +488,7 @@ local function strftime(fmt, o)
>  end
>  
>  local datetime_mt = {
> +    __tostring = datetime_tostring,
>      __serialize = datetime_serialize,
>      __eq = datetime_eq,
>      __lt = datetime_lt,
> @@ -466,6 +498,7 @@ local datetime_mt = {
>  }
>  
>  local interval_mt = {
> +    __tostring = datetime_tostring,
>      __serialize = interval_serialize,
>      __eq = datetime_eq,
>      __lt = datetime_lt,
> @@ -487,6 +520,8 @@ return setmetatable(
>          parse_time  = parse_time,
>          parse_zone  = parse_zone,
>  
> +        tostring    = datetime_tostring,
> +

I don't think we need this function in the module. Global tostring() is
enough.