On 10/08/16 05:07, Assaf Gordon wrote: > Hello Paul, Pádraig, > > Thank you for the review and suggestions. > Attached is a suggested draft patch to address some of the issues: > >> On Aug 9, 2016, at 12:32, Paul Eggert <egg...@cs.ucla.edu> wrote: >> >> These days, there's little reason to use macros instead of static functions. >> So I suggest renaming DEBUG0 to debug0 and making it a static function, and >> similarly for the other macros. > > These macros where just thin wrappers adding an "if (debug)" before calling > the actual debug functions. > In the attached patches, I removed the macros completely (each of the first 5 > changesets removes one macro, I hope they are self-explanatory). > >> parse-datetime should really be made reentrant, on systems that support >> that, by using the time_rz module. This is a larger task, though, and can be >> deferred. > > The 6th changeset replaces the global variable (parse_datetime_debug) with a > member variable in the parser_control struct. > It adds an additional function 'parse_datetime2' (temporary name) which > accepts 'bool debug' as a parameter. > By eliminating the global variable, this 'debug' feature will not make things > harder when the time comes to make the parser reentrant. > >> debug_stfdatetime can use nstrftime (in the strftime module) instead of >> doing things by hand. nstrftime deals with time zones and nanoseconds and >> should be well-behaved with unusual time stamps. > > The 7th changeset uses 'nstrftime', but not yet for the timezone, since > timezone in parse-datetime.y is stored as a long, not a 'timezone_t' . > >> Come to think of it, should we even be translating the debugging output at >> all? It's unlikely to be useful to anybody lacking access to the source >> code, which is in English. Plus, some of the strings are not being >> translated anyway, for alignment reasons. > > Regarding translation: > I hope that this 'date --debug' feature will be used similarly to 'sort > --debug' - that is, by users who are somewhat confused by the output of > 'date'. Thus, it should be self-explanatory,and should not require looking at > the source code. > By translating the messages users in other languages might be able to > diagnose problems themselves. > (Sort's debug message are also translatable). > > If we decide to remove translation, I can send additional patch for it. > > Comments very welcomed. > (Once this is decided upon, I'll send the corresponding 'date' patch to > coreutils). > > regards, > - assaf > > Assaf Gordon (7): > parse-datetime.y: remove DEBUG0 > parse-datetime.y: remove DEBUG, PROGRESS macros > parse-datetime.y: remove DEBUG_PRINT_CURRENT_TIME macro > parse-datetime.y: remove DEBUG_PRINT_RELATIVE_TIME macro > parse-datetime.y: remove DEBUG_MKTIME_NOT_OK macro > parse-datetime: refactor global debug variable to a local parameter > parse-datetime.y: use nstrftime module (instead of snprintf) > > lib/parse-datetime.h | 6 +- > lib/parse-datetime.y | 332 > ++++++++++++++++++++++++++----------------------- > modules/parse-datetime | 1 + > 3 files changed, 178 insertions(+), 161 deletions(-)
Those look good to squash and apply, modulo s/setftime/strftime/ in modules/parse-datetime thanks, Pádraig
