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 D44CD6EC58; Sun, 1 Aug 2021 20:01:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D44CD6EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627837272; bh=iFOI4WZ4kacurLmavErQ/qADtmmrwCTxZb4PWgWOUfo=; 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=zMZN7HYZzVf/bVfvdGQETczKDgxZCWpY8EbLZjGAty3sENujgMJAjtG0S7JDLe2aT OCR9hHY36te94106FFoa7zoqNPqE7zE+x2CLE2PbiuxLb8K8mSB4KDb5tCcRTl8QTu Tp9AJGuGNzxBK93Hj51hpWjr8mO/Ys5HI0E49feQ= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (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 DA9446EC58 for ; Sun, 1 Aug 2021 20:01:11 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DA9446EC58 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1mAEpq-0007Pa-Ec; Sun, 01 Aug 2021 20:01:11 +0300 To: Timur Safin References: <50175cbd00c57e3bc8eed222951a551f4bb7effb.1627468002.git.tsafin@tarantool.org> <75730675-e162-887c-3ab3-1c8a8600b27e@tarantool.org> <031c01d78559$13ae90f0$3b0bb2d0$@tarantool.org> Message-ID: <50ff0c18-b5b8-bbff-4cce-53660c39a6ce@tarantool.org> Date: Sun, 1 Aug 2021 19:01:09 +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: <031c01d78559$13ae90f0$3b0bb2d0$@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C366EE405EC28A2001F8302D8429E0DE58182A05F5380850407AD15E4BCEED99E5CEF5DFF6D3537978D8171387829E6BED1E690A7FC4034855 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7495A032B936E882FEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C1CDCB5E4A85220F8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D80ADA38C54B1510A10F81B7139FA6332F117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18BDFBBEFFF4125B51D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B613439FA09F3DCB32089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A57C3A22D95BF02AB581A4239DD857D519818B9A94ACC168BDD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA751B940EDA0DFB0535410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34BD150993ED26DDF45E37579644CE22600D7CCAF1043542F63359C82B50A638D9F8FF4A1C95D438C91D7E09C32AA3244CF85B7381DFE9BDC838A84334D15F1255A8CE788DE6831205927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojMCfuYI4PrecOXdaAp3i4RQ== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DCDE1F1E33BCB977A240E67DD86B3CFCE3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH resend v2 02/11] 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" Hi! Thanks for the fixes! Although I would prefer you sending updates right under each non-trivial comment and pushing the changes. Otherwise I can't check if you really fixed the comments and if the fixes are better than it was before. On 30.07.2021 17:39, Timur Safin wrote: > : From: Vladislav Shpilevoy > : Subject: Re: [PATCH resend v2 02/11] lua: built-in module datetime > : > : Thanks for the patch! > : > : This is a bad question to ask now, but why did you choose a dead library > : from 2015 with just 25 stars, without any tests, and without any README as > : a basic block for our datetime types? Is this the correct link I am > : using - https://github.com/chansen/c-dt? In all your responses the previous email's text is prefixed with ':' sign unlike it is done in other people's responses. In my email client it turns the email into a mess where I can't see where is the new text. Could you possibly start using '>' prefix? > : > + */ > : > +struct datetime_t { > : > + int64_t secs; ///< seconds since epoch > : > + int32_t nsec; ///< nanoseconds if any > : > + int32_t offset; ///< offset in minutes from GMT > : > : 6. We never use // nor /// nor ///< nor we write comments > : on the same line as the code (except for old legacy code we > : inherited from sqlite and some super old tarantool code. > > BTW, why? Single line comment // is the legal C construction > since C99 (and always have been acceptable by Gnu C and major > other compilers). > > Also did you know that ///< is doxygen construction for > documentation of a struct member? (Asking, just to make sure) The fact that something is a legal C construction does not mean it fits our code style. For doxygen in our rules we have /** as a comment opening. > : > : The same for datetime_interval_t. > : > : > +}; > : > + > : > +/** > : > + * Date/time delta structure > : > + */ > : > +struct datetime_interval_t { > : > + int64_t secs; ///< relative seconds delta > : > + int32_t nsec; ///< nanoseconds delta > : > +}; > : > + > : > : 7. Why do you need this file? It is not included anywhere. > : And you don't need to define the structs in C if you are > : using them in Lua only. You can just define them in Lua > : using ffi.cdef like it is done in some other places. > > I'll need to use it in C very soon, once I get to SQL parser > INTERVAL 'xx' support. You will need to introduce it in the same commit in which you will use it. We do not split patches into commits just to make them smaller. The split needs to make sense in terms of atomicity. > : > +local datetime_mt = { > : > + -- __tostring = datetime_tostring, > : > : 16. Why is it commented out? Is it even tested then? > > It's implemented in the later patch in patchset series. Well, then it should not be here in this commit. > : > + microseconds = function(self) > : > + return tonumber(self.secs*1e6 + self.nsec*1e3) > : > : 18. Please, add whitespaces around binary operators. > > Done! It was already such in the later commits which were part > of in patchset. [Yes, apparently I need to squash few more commits, > as Oleg Babin has asked elsewhere] It is not just about squashes. It is only about atomicity (and as a consequence - review simplicity). You can't just squash all the commits into one. You will need to change the older commits to make them right, then rebase the newer commits. > : > : 19. Why do you do tonumber() for all results? You are loosing > : precision for big values. The same in all the places where you > : use tonumber(). > > To be able to use it in places which expect numbers like %d in > string.format. That does not look like a good reason - you loose precision just to simplify the logs. Better make the logs use tonumber() than make the functions return imprecise results. > Though at the end of day I've came up to the version > where I convert to Lua numbers not all (potentially fraction point) > values, but with exception of nanoseconds / microseconds / milliseconds > (which return integer values). > > : > : > + end, > : > + > : > : 21. Extra empty line. > > Yup, but later, well, you know ... No, I couldn't understand. What 'later'? > : > : > +} > : > + > : > +local interval_mt = { > : > + -- __tostring = interval_tostring, > : > : 22. Ditto. Why is it commented out and not tested? > > Tostring conversion was implemented later in a few > commits. One for datetime stringization, and another > one for interval. Then it should be in the corresponding commit. > : > : > + __serialize = interval_serialize, > : > + __eq = datetime_eq, > : > + __lt = datetime_lt, > : > + __le = datetime_le, > : > +} > : > + > : > +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 local_rd(o) > : > : 23. What is 'rd' and 'dt'? > > `dt` is type - data type from c-dt (alias to signed integer). > Which shows a number of dates since Rata Die date (0001-01-01). > It's signed 32-bit value, thus could represent huge range before > 0001-01-01 as well. You didn't say what is 'rd'. > : > : > + end, > : > : 27. The usage of closures here might render all your FFI efforts > : useless, killing the performance. Please, try to define all > : methods of all objects only once in the root namespace of the > : file. Closure usage might be justified only for rarely created > : long living heavy objects like netbox. > > Agreed that creating closures in each iteration of a loop was > bad idea (as Oleg Babin has already pin-pointed it elsewhere). It is not only about in the loop. It is about having closures at all. I don't see why are they necessary here. It is not a complex state machine like netbox which needs closures. Please, rework the function not to have any closures in them. > : > + > : > +--[[ > : > + Basic Extended > : > + 20121224 2012-12-24 Calendar date (ISO 8601) > : > + 2012359 2012-359 Ordinal date (ISO 8601) > : > + 2012W521 2012-W52-1 Week date (ISO 8601) > : > + 2012Q485 2012-Q4-85 Quarter date > : > +]] > : > + > : > +local function parse_date(str) > : > + local dt = ffi.new('dt_t[1]') > : > + local rc = cdt.dt_parse_iso_date(str, #str, dt) > : > + assert(rc > 0) > : > : 29. Er ... And what if I pass a string in a wrong format? Users > : should not get assertions. > : > : tarantool> datetime.parse_date('121r1 r1r 13 13 13') > : --- > : - error: 'builtin/datetime.lua:397: assertion failed!' > : ... > : > : This shall not ever happen. Would you mind implementing proper > : error handling. The same in all the places where the errors are > : ignored. > > > Now, I've reworked approach how they handle errors (you could see > It in later commits in the patch-set), and now it returns tuple - > - parsed value > - and the number of accepted input characters. > > tarantool> date.parse_date('121r1 r1r 13 13 13') > --- > - null > - 0 > ... > > Sometimes, while parsing composed strings using these partial > methods (parse_date / parse_time / parse_time_zone) we need to > know the number of symbols we have successfully accepted. That's > why there is seconds value in returned tuple. > > Please give me know if there is better/more idiomatic approach > used for such scenarios? In the new code which is a part of the core we use exceptions. https://github.com/tarantool/doc/issues/1727 But I see it is still not documented, so for me 'null, pos' look fine so far.