From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id E0FD46EC58; Thu, 5 Aug 2021 02:59:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E0FD46EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1628121542; bh=Kf4XQTgjd3FyabdoN+rBP6Vc+cwfc65jVQCtUOA0lhU=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=OCA3Tws41zaXiJ+s7E8nf9l/FCMjhMDA2PJfAH2JvziuhRoc2JyriHKlKxE4tAGO0 7h5yWohyDU3OvcUyinKS6ZVag1jV4kNNcKSYZTGBWovujwLCh79EJuXQIr5/6MkAFD zC83KrMR7cVVJBwLT3eHwXfFIACANuwcTjuFM3EM= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4FD286EC58 for ; Thu, 5 Aug 2021 02:59:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4FD286EC58 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mBQmq-0001Gk-AH; Thu, 05 Aug 2021 02:59:00 +0300 To: Timur Safin , olegrok@tarantool.org References: Message-ID: <449cb4c7-c738-ed07-8517-61b663ad9f79@tarantool.org> Date: Thu, 5 Aug 2021 01:58:59 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD941C43E597735A9C3FDAB68B812060C77E97CF8617D978122182A05F5380850408F57D1EBD6F1FC98EA3D8CE4BDE30F805E9D207B8605994C26D4017FED7AF326 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AD2F2D6F6013FF7FC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7EA4B66823129EB3CEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F2DD0490DB060C96520863FA2C2D90E9D0CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B08F9A42B2210255C75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B5053B71511467F7217492E99B13A80AAA81 X-C1DE0DAB: 0D63561A33F958A58833555DA775E72892695DD1B02DDB02CF13493846AAAF45D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7501A9DF589746230F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3429538671E6527D32F968A50568EA29B46C17AA1200C9F7FEF01B0BA0559F76A76CF1C3721E431CB61D7E09C32AA3244C12B2C8D7A57F611F8051E0448984B90C3A92A9747B6CC88683B48618A63566E0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojh4v93/7HD3VHxC6xc6WK6Q== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DD2EBFDDA1E5A9D803A229D2566230A833841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v3 2/9] lua: built-in module datetime X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thanks for the patch! See 28 comments below. I am asking you not to rush answering 'will fix', 'ok', 'good idea' etc. It seems you are missing a lot of comments, probably because of that. You need to answer only when you already did the changes, preferably with the diff showing the change in the corresponding commit under each comment. Or when you have questions. While doing review fixed you need to keep track of the comments so as not to loose any of them. > diff --git a/src/lib/core/datetime.h b/src/lib/core/datetime.h > new file mode 100644 > index 000000000..84d6642ab > --- /dev/null > +++ b/src/lib/core/datetime.h > @@ -0,0 +1,112 @@ > +#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 ``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 > + * 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 > +#include > +#include > +#include "c-dt/dt.h" > + > +#if defined(__cplusplus) > +extern "C" > +{ > +#endif /* defined(__cplusplus) */ > + > +/** > + * Full datetime structure representing moments > + * since Unix Epoch (1970-01-01). > + * Time is kept normalized to UTC, time-zone offset > + * is informative only. > + */ > +struct datetime_t { 1. Well. How can we proceed beyond the second commit, if you keep ignoring my comments? Please, drop _t suffix. Please, comply with our code style for comments (they should not be on the same line as the members, and should not use any signs like '<'). If you did it in some later commits - it is not atomic anymore. The change must be self-sufficient. This commit is not. It depends on one of the last commits which renames something. Why couldn't you give the proper names from the beginning, from this commit? And what is worse - this entire file is not needed in this commit, I already said it before. It is simply not used here at all. Please, move it to the relevant commit. What is even further worse - you added here some functions which do not have any definition. You only added .h file. All the functions here are simply undefined. Wtf? > + int64_t secs; /**< seconds since epoch */ > + int32_t nsec; /**< nanoseconds if any */ > + int32_t offset; /**< offset in minutes from UTC */ > +}; > + > +/** > + * Date/time interval structure > + */ > +struct datetime_interval_t { > + int64_t secs; /**< relative seconds delta */ > + int32_t nsec; /**< nanoseconds delta */ > +}; > + > +int > +datetime_compare(const struct datetime_t * lhs, > + const struct datetime_t * rhs); > + > + > +struct datetime_t * > +datetime_unpack(const char **data, uint32_t len, struct datetime_t *date); > + > +/** > + * Pack datetime_t data to the MessagePack buffer. > + */ > +char * > +datetime_pack(char *data, const struct datetime_t *date); > + > +/** > + * Calculate size of MessagePack buffer for datetime_t data. > + */ > +uint32_t > +mp_sizeof_datetime(const struct datetime_t *date); > + > +/** > + * Decode data from MessagePack buffer to datetime_t structure. > + */ > +struct datetime_t * > +mp_decode_datetime(const char **data, struct datetime_t *date); > + > +/** > + * Encode datetime_t structure to the MessagePack buffer. > + */ > +char * > +mp_encode_datetime(char *data, const struct datetime_t *date); > + > +/** > + * 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_t * date, char *buf, uint32_t len); > + > +int > +mp_snprint_datetime(char *buf, int size, const char **data, uint32_t len); > + > +int > +mp_fprint_datetime(FILE *file, const char **data, uint32_t len); 2. Please, try to follow decimal and uuid here. They have separate files for working with MessagePack: tt_uuid.h/mp_uuid.h, decimal.h/mp_decimal.h. You should have datetime.h and mp_datetime.h. Also once you add these to_string, mp_fprint, mp_snprint, pack/unpack, you need to add unitests in C for all these functions. See how UUID does it in test/unit/uuid.c and decimal in test/unit/decimal.c. > diff --git a/src/lua/datetime.lua b/src/lua/datetime.lua > new file mode 100644 > index 000000000..870edc541 > --- /dev/null > +++ b/src/lua/datetime.lua <...> > + // /usr/include/x86_64-linux-gnu/bits/types/struct_tm.h > + /* ISO C `broken-down time' structure. */ > + struct tm > + { > + int tm_sec; /* Seconds. [0-60] (1 leap second) */ > + int tm_min; /* Minutes. [0-59] */ > + int tm_hour; /* Hours. [0-23] */ > + int tm_mday; /* Day. [1-31] */ > + int tm_mon; /* Month. [0-11] */ > + int tm_year; /* Year - 1900. */ > + int tm_wday; /* Day of week. [0-6] */ > + int tm_yday; /* Days in year.[0-365] */ > + int tm_isdst; /* DST. [-1/0/1]*/ > + > + long int tm_gmtoff; /* Seconds east of UTC. */ > + const char *tm_zone;/* Timezone abbreviation. */ 3. Didn't we already discuss it? The structure is platform-specific. You can't use it safely via FFI. And you even agreed with that, but still the struct is here. Why? > + }; <...> > +local datetime_t = ffi.typeof('struct datetime_t') > +local interval_t = ffi.typeof('struct datetime_interval_t') > + > +local function is_interval(o) > + return type(o) == 'cdata' and ffi.istype(interval_t, o) > +end > + > +local function is_datetime(o) > + return type(o) == 'cdata' and ffi.istype(datetime_t, o) > +end > + > +local function is_date_interval(o) > + return is_datetime(o) or is_interval(o) 4. You make type(o) == 'cdata' check 2 times here. > +end > + > +local function interval_new() > + local interval = ffi.new(interval_t) > + return interval > +end > + > +local function check_date(o, message, lvl) 5. 'lvl' is never used by any of the invocations. You waste time on the 'if', so it can be dropped. Or start using 'lvl' but never pass it as nil, please. The same for all the other functions below taking 'lvl'. > + if lvl == nil then > + lvl = 2 > + end > + if not is_datetime(o) then > + return error(("%s: expected datetime, but received %s"): > + format(message, o), lvl) > + end > +end <...> > + > +local function datetime_cmp(lhs, rhs) 6. This does not look correct: tarantool> datetime = require('datetime') --- ... tarantool> d = datetime() --- ... tarantool> d < 'bad string' --- - false ... tarantool> d > 'bad string' --- - false ... Do you have any tests? In this commit I see zero tests for your code. Only very few tests for dt library. Please, add tests for everything you have here. > + if not is_date_interval(lhs) or > + not is_date_interval(rhs) then > + return nil > + end > + local sdiff = lhs.secs - rhs.secs > + return sdiff ~= 0 and sdiff or (lhs.nsec - rhs.nsec) > +end > + > +local function datetime_eq(lhs, rhs) > + local rc = datetime_cmp(lhs, rhs) > + return rc ~= nil and rc == 0 or false > +end > + > +local function datetime_lt(lhs, rhs) > + local rc = datetime_cmp(lhs, rhs) > + return rc ~= nil and rc < 0 or false > +end > + > +local function datetime_le(lhs, rhs) > + local rc = datetime_cmp(lhs, rhs) > + return rc ~= nil and rc <= 0 or false > +end > + > +local function datetime_serialize(self) > + -- Allow YAML, MsgPack and JSON to dump objects with sockets > + return { secs = self.secs, nsec = self.nsec, offset = self.offset } > +end > + > +local function interval_serialize(self) > + -- Allow YAML and JSON to dump objects with sockets 7. What do these comments mean? Why is it specific to these formats? And why doesn this comment include MsgPack while the previous one does? > + return { secs = self.secs, nsec = self.nsec } > +end > + > +local function local_rd(o) > + return math.floor(tonumber(o.secs / SECS_PER_DAY)) + DT_EPOCH_1970_OFFSET 8. I suggest to cache math.floor into a global variable. The same for the other math.* usages in this file. To save one index operation. > +end > + > +local function local_dt(o) > + return builtin.dt_from_rdn(local_rd(o)) > +end > + > +local function _normalize_nsec(secs, nsec) 9. Why do you have '_' prefix? The function is not global, you don't need any protection against name conflicts. > + if nsec < 0 then > + secs = secs - 1 > + nsec = nsec + NANOS_PER_SEC > + elseif nsec >= NANOS_PER_SEC then > + secs = secs + 1 > + nsec = nsec - NANOS_PER_SEC > + end > + return secs, nsec > +end > + > +local function check_range(v, range, txt) > + assert(#range == 2) > + if not (v >= range[1] and v <= range[2]) then 10. Wouldn't it be shorter to make if v < range[1] or v > range[2] then ? > + error(('value %d of %s is out of allowed range [%d, %d]'): > + format(v, txt, range[1], range[2])) > + end > +end > + > +local datetime_index_handlers = { > + unixtime = function(self) > + return self.secs > + end, > + > + timestamp = function(self) > + return tonumber(self.secs) + self.nsec / 1e9 11. If you are saying the Lua number value range is enough, why do you store them as 64bit integers in 'self' and convert to a number only for the external API? > + end, > + > + nanoseconds = function(self) > + return self.secs * 1e9 + self.nsec > + end, > + > + microseconds = function(self) > + return self.secs * 1e6 + self.nsec / 1e3 > + end, > + > + milliseconds = function(self) > + return self.secs * 1e3 + self.nsec / 1e6 > + end, > + > + seconds = function(self) > + return tonumber(self.secs) + self.nsec / 1e9 > + end, > + > + minutes = function(self) > + return (tonumber(self.secs) + self.nsec / 1e9) / 60 > + end, > + > + hours = function(self) > + return (tonumber(self.secs) + self.nsec / 1e9) / (60 * 60) > + end, > + > + days = function(self) > + return (tonumber(self.secs) + self.nsec / 1e9) / (60 * 60) / 24 12. Is Lua parser able to merge the last constants into one number to make only one division at runtime? If not, I suggest to do it manually. > + end, > +} > + > +local datetime_index = function(self, key) > + return datetime_index_handlers[key] ~= nil and > + datetime_index_handlers[key](self) or nil 13. To save one hash lookup operation you can do this: local h = datetime_index_handlers[key] return h and h(self) The same for datetime_newindex. > +end > + > +local datetime_newindex_handlers = { > + unixtime = function(self, value) > + self.secs = value > + self.nsec, self.offset = 0, 0 > + end, > + > + timestamp = function(self, value) > + local secs, frac = math.modf(value) > + self.secs = secs > + self.nsec = frac * 1e9 > + self.offset = 0 > + end, > +} > + > +local function datetime_newindex(self, key, value) > + if datetime_newindex_handlers[key] ~= nil then > + datetime_newindex_handlers[key](self, value) > + end > +end > + > +local function datetime_new_raw(secs, nsec, offset) > + local dt_obj = ffi.new(datetime_t) > + dt_obj.secs = secs > + dt_obj.nsec = nsec > + dt_obj.offset = offset > + return dt_obj > +end > + > +local function mk_timestamp(dt, sp, fp, offset) 14. What are 'sp' and 'fp'? 15. The function is a datetime constructor, because it creates and returns a new datetime object. Please, call it correspondingly then. For instance, `datetime_new_dt()`. > + local epochV = dt ~= nil and (builtin.dt_rdn(dt) - DT_EPOCH_1970_OFFSET) * > + SECS_PER_DAY or 0 > + local spV = sp ~= nil and sp or 0 > + local fpV = fp ~= nil and fp or 0 > + local ofsV = offset ~= nil and offset or 0 > + return datetime_new_raw (epochV + spV - ofsV * 60, fpV, ofsV) > +end > + > +-- create @datetime_t given object @o fields 16. Didn't we agree in the previous review that you will stop using name 'o'? At least in the context where there are zeros. It is very confusing. > +local function datetime_new(o) > + if o == nil then > + return datetime_new_raw(0, 0, 0) > + end > + local secs = 0 > + local nsec = 0 > + local offset = 0 > + local easy_way = false > + local y = 0 > + local M = 0 > + local d = 0 > + local ymd = false > + > + local h = 0 > + local m = 0 > + local s = 0 > + local frac = 0 > + local hms = false > + > + local dt = 0 > + > + local handlers = { 17. Please, stop using closures in the constructor of a potentionally very frequently created/deleted object. > + secs = function(_, v) > + secs = v > + easy_way = true > + end, > + > + nsec = function(_, v) > + nsec = v > + easy_way = true > + end, > + > + offset = function (_, v) > + offset = v > + easy_way = true > + end, > + > + year = function(k, v) > + check_range(v, {1, 9999}, k) > + y = v > + ymd = true > + end, > + > + month = function(k, v) > + check_range(v, {1, 12}, k) > + M = v > + ymd = true > + end, > + > + day = function(k, v) > + check_range(v, {1, 31}, k) > + d = v > + ymd = true > + end, > + > + hour = function(k, v) > + check_range(v, {0, 23}, k) > + h = v > + hms = true > + end, > + > + minute = function(k, v) > + check_range(v, {0, 59}, k) > + m = v > + hms = true > + end, > + > + second = function(k, v) > + check_range(v, {0, 60}, k) > + s, frac = math.modf(v) > + frac = frac * 1e9 -- convert fraction to nanoseconds > + hms = true > + end, > + > + -- tz offset in minutes > + tz = function(k, v) > + check_range(v, {0, 720}, k) > + offset = v > + end > + } > + for key, value in pairs(o) do > + handlers[key](key, value) > + end > + > + -- .sec, .nsec, .offset > + if easy_way then > + return datetime_new_raw(secs, nsec, offset) > + end > + > + -- .year, .month, .day > + if ymd then > + dt = dt + builtin.dt_from_ymd(y, M, d) > + end > + > + -- .hour, .minute, .second > + if hms then > + secs = h * 3600 + m * 60 + s > + end > + > + return mk_timestamp(dt, secs, frac, offset) > +end 18. It fails with a bad error: tarantool> d = datetime({garbage = true}) --- - error: 'builtin/datetime.lua:449: attempt to call a nil value' ... 19. It can ignore some fields (here it ignores 'year'): tarantool> d1 = datetime({secs = 100, year = 1}) --- ... tarantool> d2 = datetime({secs = 100, year = 2}) --- ... tarantool> d1 == d2 --- - true ... 20. Serialization does not seem to be working in the console despite you tried to implement it above: tarantool> d = datetime({secs = 100}) --- ... tarantool> d --- - 'cdata: 0x0103ad9c50' ... > + > +local function date_first(lhs, rhs) > + if is_datetime(lhs) then > + return lhs, rhs > + else > + return rhs, lhs > + end > +end > + > +local function error_incompatible(name) > + error(("datetime:%s() - incompatible type of arguments"): > + format(name), 3) > +end > + > +local function datetime_sub(lhs, rhs) > + check_date_interval(lhs, "operator -") > + local d, s = lhs, rhs > + local left_t = ffi.typeof(d) > + local right_t = ffi.typeof(s) > + local o > + > + if left_t == datetime_t then > + -- left is date, right is date or generic interval > + if (right_t == datetime_t or right_t == interval_t) then 21. You do not need () around the condition in Lua. > + o = right_t == datetime_t and interval_new() or datetime_new() > + o.secs, o.nsec = _normalize_nsec(lhs.secs - rhs.secs, > + lhs.nsec - rhs.nsec) > + return o > + else > + error_incompatible("operator -") > + end > + -- both left and right are generic intervals > + elseif left_t == interval_t and right_t == interval_t then > + o = interval_new() > + o.secs, o.nsec = _normalize_nsec(lhs.secs - rhs.secs, > + lhs.nsec - rhs.nsec) > + return o > + else > + error_incompatible("operator -") > + end > +end 22. Did you really test this function? tarantool> dt = require('datetime') --- ... tarantool> d1 = dt() --- ... tarantool> d2 - 1 --- - error: 'builtin/datetime.lua:487: bad argument #1 to ''typeof'' (C type expected, got number)' ... > + > +local function datetime_add(lhs, rhs) > + local d, s = date_first(lhs, rhs) > + > + check_date_interval(d, "operator +") > + check_interval(s, "operator +") > + local left_t = ffi.typeof(d) > + local right_t = ffi.typeof(s) > + local o > + > + -- left is date, right is date or interval > + if left_t == datetime_t and right_t == interval_t then > + o = datetime_new() > + o.secs, o.nsec = _normalize_nsec(d.secs + s.secs, d.nsec + s.nsec) > + return o > + -- both left and right are generic intervals > + elseif left_t == interval_t and right_t == interval_t then > + o = interval_new() > + o.secs, o.nsec = _normalize_nsec(d.secs + s.secs, d.nsec + s.nsec) > + return o > + else > + error_incompatible("operator +") > + end > +end > + > +-- simple parse functions: > +-- parse_date/parse_time/parse_zone 23. As I said in the previous review, the comment is useless. You just listed 3 function names following the comment. What is the purpose of that? What should I learn from it? Please, drop it. <...> > + > +local function datetime_from(o) > + if o == nil or type(o) == 'table' then > + return datetime_new(o) > + elseif type(o) == 'string' then > + return parse(o) > + end > +end > + > +local function local_now() > + local p_tv = ffi.new('struct timeval [1]') > + local rc = builtin.gettimeofday(p_tv, nil) 24. Why can't you use fiber time? It should be enough in nearly all usecases and is much faster. For real-real time talking directly to the kernel we have 'clock' Lua module already. You could check if results of clock functions can be passed into your module to create 'real' timestamps not depending on the eventloop. > + assert(rc == 0) > + > + local secs = p_tv[0].tv_sec > + local nsec = p_tv[0].tv_usec * 1000 > + > + local p_time = ffi.new('time_t[1]') > + local p_tm = ffi.new('struct tm[1]') > + builtin.time(p_time) > + builtin.localtime_r(p_time, p_tm) > + local ofs = p_tm[0].tm_gmtoff / 60 -- convert seconds to minutes > + > + return datetime_new_raw(secs, nsec, ofs) > +end > + > +local function datetime_to_tm_ptr(o) > + assert(is_datetime(o)) > + local p_tm = ffi.new('struct tm[1]') > + -- dt_to_struct_tm() fills only date data > + builtin.dt_to_struct_tm(local_dt(o), p_tm) > + > + -- calculate the smaller data (hour, minute, > + -- seconds) using datetime seconds value > + local seconds_of_day = o.secs % 86400 > + local hour = (seconds_of_day / 3600) % 24 > + local minute = (seconds_of_day / 60) % 60 > + p_tm[0].tm_sec = seconds_of_day % 60 > + p_tm[0].tm_min = minute > + p_tm[0].tm_hour = hour > + > + p_tm[0].tm_gmtoff = o.offset * 60 > + > + return p_tm > +end > + > +local function asctime(o) > + check_date(o, "datetime:asctime()") > + > + local p_tm = datetime_to_tm_ptr(o) > + return ffi.string(builtin.asctime(p_tm)) > +end > + > +local function ctime(o) > + check_date(o, "datetime:ctime()") > + local p_time = ffi.new('time_t[1]') > + p_time[0] = o.secs > + return ffi.string(builtin.ctime(p_time)) 25. Functions like ctime can't be used in Lua, because according to their Linux man: The return value points to a statically allocated string which might be overwritten by subsequent calls to any of the date and time functions Which means they use a global buffer. If you will use datetime module in any of __gc handlers, it will return garbage sometimes or just override results somethere. That is exactly the same problem as existed with tarantool's static buffer. > diff --git a/test/unit/datetime.c b/test/unit/datetime.c > new file mode 100644 > index 000000000..b6f568c03 > --- /dev/null > +++ b/test/unit/datetime.c 26. This file tests dt library. It does not test your code. Please, add tests for everything you implemented in this commit. For c-dt library the tests must be added in the previous commit, and the test file must be called test/unit/cdt.c then. > @@ -0,0 +1,221 @@ > +#include "dt.h" > +#include > +#include > +#include > +#include > + > +#include "unit.h" > + > +const char sample[] = "2012-12-24T15:30Z"; 27. Please, make it static. > + > +#define S(s) {s, sizeof(s) - 1} > +struct { > + const char * sz; > + size_t len; > +} tests[] = { > + S("2012-12-24 15:30Z"), > + S("2012-12-24 15:30z"), > + S("2012-12-24 15:30"), > + S("2012-12-24 16:30+01:00"), > + S("2012-12-24 16:30+0100"), > + S("2012-12-24 16:30+01"), > + S("2012-12-24 14:30-01:00"), > + S("2012-12-24 14:30-0100"), > + S("2012-12-24 14:30-01"), > + S("2012-12-24 15:30:00Z"), > + S("2012-12-24 15:30:00z"), > + S("2012-12-24 15:30:00"), > + S("2012-12-24 16:30:00+01:00"), > + S("2012-12-24 16:30:00+0100"), > + S("2012-12-24 14:30:00-01:00"), > + S("2012-12-24 14:30:00-0100"), > + S("2012-12-24 15:30:00.123456Z"), > + S("2012-12-24 15:30:00.123456z"), > + S("2012-12-24 15:30:00.123456"), > + S("2012-12-24 16:30:00.123456+01:00"), > + S("2012-12-24 16:30:00.123456+01"), > + S("2012-12-24 14:30:00.123456-01:00"), > + S("2012-12-24 14:30:00.123456-01"), > + S("2012-12-24t15:30Z"), > + S("2012-12-24t15:30z"), > + S("2012-12-24t15:30"), > + S("2012-12-24t16:30+01:00"), > + S("2012-12-24t16:30+0100"), > + S("2012-12-24t14:30-01:00"), > + S("2012-12-24t14:30-0100"), > + S("2012-12-24t15:30:00Z"), > + S("2012-12-24t15:30:00z"), > + S("2012-12-24t15:30:00"), > + S("2012-12-24t16:30:00+01:00"), > + S("2012-12-24t16:30:00+0100"), > + S("2012-12-24t14:30:00-01:00"), > + S("2012-12-24t14:30:00-0100"), > + S("2012-12-24t15:30:00.123456Z"), > + S("2012-12-24t15:30:00.123456z"), > + S("2012-12-24t16:30:00.123456+01:00"), > + S("2012-12-24t14:30:00.123456-01:00"), > + S("2012-12-24 16:30 +01:00"), > + S("2012-12-24 14:30 -01:00"), > + S("2012-12-24 15:30 UTC"), > + S("2012-12-24 16:30 UTC+1"), > + S("2012-12-24 16:30 UTC+01"), > + S("2012-12-24 16:30 UTC+0100"), > + S("2012-12-24 16:30 UTC+01:00"), > + S("2012-12-24 14:30 UTC-1"), > + S("2012-12-24 14:30 UTC-01"), > + S("2012-12-24 14:30 UTC-01:00"), > + S("2012-12-24 14:30 UTC-0100"), > + S("2012-12-24 15:30 GMT"), > + S("2012-12-24 16:30 GMT+1"), > + S("2012-12-24 16:30 GMT+01"), > + S("2012-12-24 16:30 GMT+0100"), > + S("2012-12-24 16:30 GMT+01:00"), > + S("2012-12-24 14:30 GMT-1"), > + S("2012-12-24 14:30 GMT-01"), > + S("2012-12-24 14:30 GMT-01:00"), > + S("2012-12-24 14:30 GMT-0100"), > + S("2012-12-24 14:30 -01:00"), > + S("2012-12-24 16:30:00 +01:00"), > + S("2012-12-24 14:30:00 -01:00"), > + S("2012-12-24 16:30:00.123456 +01:00"), > + S("2012-12-24 14:30:00.123456 -01:00"), > + S("2012-12-24 15:30:00.123456 -00:00"), > + S("20121224T1630+01:00"), > + S("2012-12-24T1630+01:00"), > + S("20121224T16:30+01"), > + S("20121224T16:30 +01"), > +}; > +#undef S > + > +#define DIM(a) (sizeof(a) / sizeof(a[0])) > + > +// p5-time-moment/src/moment_parse.c: parse_string_lenient() 28. Please, stop using // comment opening.