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

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

On 17.08.2021 20:06, Vladimir Davydov via Tarantool-patches wrote:
> 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)

AFAIK external comments (in headers) should be doxygen, but internal 
comments (in the implementation) may be not in doxygen format. That's 
internal comment.

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

Yes, already has have it reimplemented after Serge Petrenko comment.
It's a little bit shorter now. Nice!

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

Done.

> 
>> +	sec = secs % 60;
> 
> sec, secs, oh
> 
> Please use better names.

Renamed.

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

Fixed.

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

Will reorder.

[BTW, if we keep argument at the same order then for x64 ABI, with 
fastcall and upto 6 arguments passed via registers RDI, RSI, RDX, RCX, 
R8, R9, then compiler may keep arguments in the same registers without 
any spilling to the stack]

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

Documented.

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

Yes, that will simplify. Part of a corresponding diff below...
----------------------------------------
@@ -373,9 +366,16 @@ local function datetime_new(obj)
          secs = h * 3600 + m * 60 + s
      end

-    return datetime_new_dt(dt, secs, frac, offset)
+    return datetime_new_dt(dt, secs, nsec, offset)
  end

+--[[
+    Convert to text datetime values
+
+    - datetime will use ISO-8601 forat:
+        1970-01-01T00:00Z
+        2021-08-18T16:57:08.981725+03:00
+]]
  local function datetime_tostring(o)
      if ffi.typeof(o) == datetime_t then
          local sz = 48
@@ -383,7 +383,25 @@ local function datetime_tostring(o)
          local len = builtin.datetime_to_string(o, buff, sz)
          assert(len < sz)
          return ffi.string(buff)
-    elseif ffi.typeof(o) == interval_years_t then
+    end
+end
+
+--[[
+    Convert to text interval values of different types
+
+    - depending on a values stored there generic interval
+      values may display in following format:
+        +12 secs
+        -23 minutes, 0 seconds
+        +12 hours, 23 minutes, 1 seconds
+        -7 days, 23 hours, 23 minutes, 1 seconds
+    - years will be displayed as
+        +10 years
+    - months will be displayed as:
+         +2 months
+]]
+local function interval_tostring(o)
+    if ffi.typeof(o) == interval_years_t then
          return ('%+d years'):format(o.y)
      elseif ffi.typeof(o) == interval_months_t then
          return ('%+d months'):format(o.m)
@@ -828,7 +901,7 @@ local datetime_mt = {
  }

  local interval_mt = {
-    __tostring = datetime_tostring,
+    __tostring = interval_tostring,
      __serialize = interval_serialize,
      __eq = datetime_eq,
      __lt = datetime_lt,
@@ -839,7 +912,7 @@ local interval_mt = {
  }

  local interval_tiny_mt = {
-    __tostring = datetime_tostring,
+    __tostring = interval_tostring,
      __serialize = interval_serialize,
      __sub = datetime_sub,
      __add = datetime_add,
----------------------------------------
> 
>> +        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.
> 

-----------------------------------------
@@ -862,15 +935,12 @@ return setmetatable(
          hours       = interval_hours_new,
          minutes     = interval_minutes_new,
          seconds     = interval_seconds_new,
-        interval    = interval_new,

          parse       = parse,
          parse_date  = parse_date,
          parse_time  = parse_time,
          parse_zone  = parse_zone,

-        tostring    = datetime_tostring,
-
          now         = local_now,
          strftime    = strftime,
          asctime     = asctime,
-----------------------------------------

Deleted.

Thanks,
Timur