* [PATCH] Add support for TZ names with <> in tzset [not found] <080401d6bada$6c52f060$44f8d120$.ref@yahoo.com> @ 2020-11-14 23:03 ` earlephilhower 2020-11-16 15:13 ` Corinna Vinschen 0 siblings, 1 reply; 8+ messages in thread From: earlephilhower @ 2020-11-14 23:03 UTC (permalink / raw) To: newlib Howdy all, Attached is a patch which extends the tzset() function to support a format for "unnamed" TZ environment timezones which use "<+/-nn>" as the timezone name instead of an alphabetic name. These are supported in glibc and are present in several major TZ databases that we use on the ESP8266 Arduino core. For example, > #define TZ_Africa_Casablanca "<+01>-1" The existing tzset sscanf format string breaks at the first "+", assuming it's the beginning of the offset. This patch special-cases names beginning with "<" to circumvent the issue. Signed-off-by: Earle F. Philhower, III <earlephilhower@yahoo.com> --- newlib/libc/time/tzset_r.c | 46 ++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/newlib/libc/time/tzset_r.c b/newlib/libc/time/tzset_r.c index 9e0cf834b..5b8b692ee 100644 --- a/newlib/libc/time/tzset_r.c +++ b/newlib/libc/time/tzset_r.c @@ -45,8 +45,19 @@ _tzset_unlocked_r (struct _reent *reent_ptr) if (*tzenv == ':') ++tzenv; - if (sscanf (tzenv, "%10[^0-9,+-]%n", __tzname_std, &n) <= 0) - return; + if (tzenv[0] == '<') + { + /* This is of the form "<[+-]nn>" so needs a different parsing */ + if (sscanf (tzenv, "%9[^>]>%n", __tzname_std, &n) <= 0) + return; + /* Include the final > */ + strcat (__tzname_std, ">"); + } + else + { + if (sscanf (tzenv, "%10[^0-9,+-]%n", __tzname_std, &n) <= 0) + return; + } tzenv += n; @@ -69,15 +80,32 @@ _tzset_unlocked_r (struct _reent *reent_ptr) _tzname[0] = __tzname_std; tzenv += n; - if (sscanf (tzenv, "%10[^0-9,+-]%n", __tzname_dst, &n) <= 0) - { /* No dst */ - _tzname[1] = _tzname[0]; - _timezone = tz->__tzrule[0].offset; - _daylight = 0; - return; + if (tzenv[0] == '<') + { + /* This is of the form "<[+-]nn>" so needs a different parsing */ + if (sscanf (tzenv, "%9[^>]>%n", __tzname_dst, &n) <= 0) + { /* No dst */ + _tzname[1] = _tzname[0]; + _timezone = tz->__tzrule[0].offset; + _daylight = 0; + return; + } + /* Include the final > */ + strcat (__tzname_dst, ">"); + _tzname[1] = __tzname_dst; } else - _tzname[1] = __tzname_dst; + { + if (sscanf (tzenv, "%10[^0-9,+-]%n", __tzname_dst, &n) <= 0) + { /* No dst */ + _tzname[1] = _tzname[0]; + _timezone = tz->__tzrule[0].offset; + _daylight = 0; + return; + } + else + _tzname[1] = __tzname_dst; + } tzenv += n; -- 2.17.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Add support for TZ names with <> in tzset 2020-11-14 23:03 ` [PATCH] Add support for TZ names with <> in tzset earlephilhower @ 2020-11-16 15:13 ` Corinna Vinschen 2020-11-16 22:30 ` Brian Inglis 0 siblings, 1 reply; 8+ messages in thread From: Corinna Vinschen @ 2020-11-16 15:13 UTC (permalink / raw) To: Earle F. Philhower, III; +Cc: newlib Hi Earle, On Nov 14 15:03, Earle F. Philhower, III via Newlib wrote: > Howdy all, > > Attached is a patch which extends the tzset() function to support a format > for "unnamed" TZ environment timezones which use "<+/-nn>" as the timezone > name instead of an alphabetic name. These are supported in glibc and are > present in several major TZ databases that we use on the ESP8266 Arduino > core. For example, > > > #define TZ_Africa_Casablanca "<+01>-1" > > The existing tzset sscanf format string breaks at the first "+", assuming > it's the > beginning of the offset. This patch special-cases names beginning with "<" > to > circumvent the issue. > > Signed-off-by: Earle F. Philhower, III <earlephilhower@yahoo.com> Basically this looks ok. I have two nits, though. - Now that the scanning got more complicated than a single sscanf call, this crys out for a helper function doing the actual scanning for both, std and dst strings. This could be an inline function which is only inlined #if !defined(PREFER_SIZE_OVER_SPEED) && !defined(__OPTIMIZE_SIZE__) - The strcat call seems a bit heavy. What about sth like this instead: __tzname_ptr[n - 1] = '>'; __tzname_ptr[n] = '\0'; Thanks, Corinna > --- > newlib/libc/time/tzset_r.c | 46 ++++++++++++++++++++++++++++++-------- > 1 file changed, 37 insertions(+), 9 deletions(-) > > diff --git a/newlib/libc/time/tzset_r.c b/newlib/libc/time/tzset_r.c > index 9e0cf834b..5b8b692ee 100644 > --- a/newlib/libc/time/tzset_r.c > +++ b/newlib/libc/time/tzset_r.c > @@ -45,8 +45,19 @@ _tzset_unlocked_r (struct _reent *reent_ptr) > if (*tzenv == ':') > ++tzenv; > > - if (sscanf (tzenv, "%10[^0-9,+-]%n", __tzname_std, &n) <= 0) > - return; > + if (tzenv[0] == '<') > + { > + /* This is of the form "<[+-]nn>" so needs a different parsing */ > + if (sscanf (tzenv, "%9[^>]>%n", __tzname_std, &n) <= 0) > + return; > + /* Include the final > */ > + strcat (__tzname_std, ">"); > + } > + else > + { > + if (sscanf (tzenv, "%10[^0-9,+-]%n", __tzname_std, &n) <= 0) > + return; > + } > > tzenv += n; > > @@ -69,15 +80,32 @@ _tzset_unlocked_r (struct _reent *reent_ptr) > _tzname[0] = __tzname_std; > tzenv += n; > > - if (sscanf (tzenv, "%10[^0-9,+-]%n", __tzname_dst, &n) <= 0) > - { /* No dst */ > - _tzname[1] = _tzname[0]; > - _timezone = tz->__tzrule[0].offset; > - _daylight = 0; > - return; > + if (tzenv[0] == '<') > + { > + /* This is of the form "<[+-]nn>" so needs a different parsing */ > + if (sscanf (tzenv, "%9[^>]>%n", __tzname_dst, &n) <= 0) > + { /* No dst */ > + _tzname[1] = _tzname[0]; > + _timezone = tz->__tzrule[0].offset; > + _daylight = 0; > + return; > + } > + /* Include the final > */ > + strcat (__tzname_dst, ">"); > + _tzname[1] = __tzname_dst; > } > else > - _tzname[1] = __tzname_dst; > + { > + if (sscanf (tzenv, "%10[^0-9,+-]%n", __tzname_dst, &n) <= 0) > + { /* No dst */ > + _tzname[1] = _tzname[0]; > + _timezone = tz->__tzrule[0].offset; > + _daylight = 0; > + return; > + } > + else > + _tzname[1] = __tzname_dst; > + } > > tzenv += n; > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Add support for TZ names with <> in tzset 2020-11-16 15:13 ` Corinna Vinschen @ 2020-11-16 22:30 ` Brian Inglis [not found] ` <DM2P110MB01699F833CC488133A5AE9E99AE20@DM2P110MB0169.NAMP110.PROD.OUTLOOK.COM> 0 siblings, 1 reply; 8+ messages in thread From: Brian Inglis @ 2020-11-16 22:30 UTC (permalink / raw) To: newlib On 2020-11-16 08:13, Corinna Vinschen via Newlib wrote: > Hi Earle, > > On Nov 14 15:03, Earle F. Philhower, III via Newlib wrote: >> Howdy all, >> >> Attached is a patch which extends the tzset() function to support a format >> for "unnamed" TZ environment timezones which use "<+/-nn>" as the timezone >> name instead of an alphabetic name. These are supported in glibc and are >> present in several major TZ databases that we use on the ESP8266 Arduino >> core. For example, >> >>> #define TZ_Africa_Casablanca "<+01>-1" >> >> The existing tzset sscanf format string breaks at the first "+", assuming >> it's the >> beginning of the offset. This patch special-cases names beginning with "<" >> to >> circumvent the issue. >> >> Signed-off-by: Earle F. Philhower, III <earlephilhower@yahoo.com> > > Basically this looks ok. I have two nits, though. > > - Now that the scanning got more complicated than a single sscanf call, > this crys out for a helper function doing the actual scanning for > both, std and dst strings. This could be an inline function which is > only inlined > #if !defined(PREFER_SIZE_OVER_SPEED) && !defined(__OPTIMIZE_SIZE__) > > - The strcat call seems a bit heavy. What about sth like this instead: > > __tzname_ptr[n - 1] = '>'; > __tzname_ptr[n] = '\0'; Should consider modifying the PD TZ project tzcode reference src localtime.c tzparse() https://github.com/eggert/tz/blob/master/localtime.c#L1069 (contributed to the public domain by Guy Harris): handles all the POSIX rules string edge cases to Paul Eggert's satisfaction, and gets patched if any upstream org (every distro) or vendor (all OSes) reports a problem. -- Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada This email may be disturbing to some readers as it contains too much technical detail. Reader discretion is advised. [Data in binary units and prefixes, physical quantities in SI.] ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <DM2P110MB01699F833CC488133A5AE9E99AE20@DM2P110MB0169.NAMP110.PROD.OUTLOOK.COM>]
* Re: Fw: [PATCH] Add support for TZ names with <> in tzset [not found] ` <DM2P110MB01699F833CC488133A5AE9E99AE20@DM2P110MB0169.NAMP110.PROD.OUTLOOK.COM> @ 2020-11-17 1:43 ` C Howland 2020-11-17 4:59 ` Brian Inglis 0 siblings, 1 reply; 8+ messages in thread From: C Howland @ 2020-11-17 1:43 UTC (permalink / raw) To: newlib ------------------------------ > *From:* Newlib <newlib-bounces@sourceware.org> on behalf of Brian Inglis < > Brian.Inglis@SystematicSw.ab.ca> > *Sent:* Monday, November 16, 2020 5:30 PM > *To:* newlib@sourceware.org <newlib@sourceware.org> > *Subject:* Re: [PATCH] Add support for TZ names with <> in tzset > > On 2020-11-16 08:13, Corinna Vinschen via Newlib wrote: > > Hi Earle, > > > > On Nov 14 15:03, Earle F. Philhower, III via Newlib wrote: > >> Howdy all, > >> > >> Attached is a patch which extends the tzset() function to support a > format > >> for "unnamed" TZ environment timezones which use "<+/-nn>" as the > timezone > >> name instead of an alphabetic name. These are supported in glibc and > are > >> present in several major TZ databases that we use on the ESP8266 Arduino > >> core. For example, > >> > >>> #define TZ_Africa_Casablanca "<+01>-1" > >> > >> The existing tzset sscanf format string breaks at the first "+", > assuming > >> it's the > >> beginning of the offset. This patch special-cases names beginning with > "<" > >> to > >> circumvent the issue. > >> > >> Signed-off-by: Earle F. Philhower, III <earlephilhower@yahoo.com> > > > > Basically this looks ok. I have two nits, though. > > > > - Now that the scanning got more complicated than a single sscanf call, > > this crys out for a helper function doing the actual scanning for > > both, std and dst strings. This could be an inline function which is > > only inlined > > #if !defined(PREFER_SIZE_OVER_SPEED) && !defined(__OPTIMIZE_SIZE__) > > > > - The strcat call seems a bit heavy. What about sth like this instead: > > > > __tzname_ptr[n - 1] = '>'; > > __tzname_ptr[n] = '\0'; > > Should consider modifying the PD TZ project tzcode reference src > localtime.c > tzparse() > > https://github.com/eggert/tz/blob/master/localtime.c#L1069 > > (contributed to the public domain by Guy Harris): handles all the POSIX > rules > string edge cases to Paul Eggert's satisfaction, and gets patched if any > upstream org (every distro) or vendor (all OSes) reports a problem. > > -- > Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada > The proposal is missing the documentation update (in tzset.c) for the new feature. (This of course is needed regardless of the eventual method by which it is achieved.) Also, one question: since at present the TZ string is matching POSIX (at least, it is according to the documentation--I did not check the present source code against the statement), shouldn't there be a gate to control whether the extension is included or not? (In my tzset(3) man page in RHEL7, GLIBC does not mention this particular extension, so I don't know what gate they might use.) Strictly there should be; the question is how strict we want to be. Craig ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Add support for TZ names with <> in tzset 2020-11-17 1:43 ` Fw: " C Howland @ 2020-11-17 4:59 ` Brian Inglis 2020-12-12 17:56 ` Earle F. Philhower, III 0 siblings, 1 reply; 8+ messages in thread From: Brian Inglis @ 2020-11-17 4:59 UTC (permalink / raw) To: newlib On 2020-11-16 18:43, C Howland via Newlib wrote: > On Monday, November 16, 2020 5:30 PM, Brian Inglis wrote: >> On 2020-11-16 08:13, Corinna Vinschen via Newlib wrote: >>> On Nov 14 15:03, Earle F. Philhower, III via Newlib wrote: >>>> Attached is a patch which extends the tzset() function to support a >>>> format for "unnamed" TZ environment timezones which use "<+/-nn>" as >>>> the timezone name instead of an alphabetic name. These are supported >>>> in glibc and are present in several major TZ databases that we use on >>>> the ESP8266 Arduino core. For example, >>>> >>>> #define TZ_Africa_Casablanca "<+01>-1" >>>> >>>> The existing tzset sscanf format string breaks at the first "+", >>>> assuming it's the beginning of the offset. This patch special-cases >>>> names beginning with "<" to circumvent the issue. >>>> >>> Basically this looks ok. I have two nits, though. >>> >>> - Now that the scanning got more complicated than a single sscanf call, >>> this crys out for a helper function doing the actual scanning for >>> both, std and dst strings. This could be an inline function which is >>> only inlined >>> #if !defined(PREFER_SIZE_OVER_SPEED) && !defined(__OPTIMIZE_SIZE__) >>> >>> - The strcat call seems a bit heavy. What about sth like this instead: >>> >>> __tzname_ptr[n - 1] = '>'; >>> __tzname_ptr[n] = '\0'; >> Should consider modifying the PD TZ project tzcode reference src >> localtime.c tzparse() >> >> https://github.com/eggert/tz/blob/master/localtime.c#L1069 >> >> (contributed to the public domain by Guy Harris): handles all the POSIX >> rules string edge cases to Paul Eggert's satisfaction, and gets patched if >> any upstream org (every distro) or vendor (all OSes) reports a problem. > The proposal is missing the documentation update (in tzset.c) for the new > feature. (This of course is needed regardless of the eventual method by > which it is achieved.) > Also, one question: since at present the TZ string is matching POSIX (at > least, it is according to the documentation--I did not check the present > source code against the statement), shouldn't there be a gate to control > whether the extension is included or not? (In my tzset(3) man page in RHEL7, > GLIBC does not mention this particular extension, so I don't know what gate > they might use.) Strictly there should be; the question is how strict we > want to be. The <> quoted format has been a POSIX standard since Issue 6 IEEE Std 1003.1, 2004; see under Base Definitions, 8.3 Other Environment Variables, bottom of web page, under TZ: https://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.htmltag_08_03 also Rationale, A.8.3 Other Environment Variables, bottom of web page, under TZ: https://pubs.opengroup.org/onlinepubs/009695399/xrat/xbd_chap08.html#tag_01_08_03_05 and not an extension, intended to allow specification of signed alpha/numeric character time zone designations, not just the original alphabetic character designations, with at least 3 not more than TZNAME_MAX (minimum _POSIX_TZNAME_MAX == 6) characters. The POSIX TZ env var current spec is essentially unchanged under Base Definitions, 8.3 Other Environment Variables, bottom of web page, under TZ: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03 also Rationale, A.8.3 Other Environment Variables, bottom of web page, under TZ: https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap08.html#tag_21_08_03_05 where the leading : for implementation defined zones is treated as optional by implementations supporting zoneinfo TZif files, and better implementations will handle relative paths specially (normally prefix with TZDIR, or at least start by trying that location, then try alternatives), and allow and handle absolute paths in the obvious way. Perhaps Eric Blake if available could shed more light as a POSIX contributor. -- Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada This email may be disturbing to some readers as it contains too much technical detail. Reader discretion is advised. [Data in binary units and prefixes, physical quantities in SI.] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Add support for TZ names with <> in tzset 2020-11-17 4:59 ` Brian Inglis @ 2020-12-12 17:56 ` Earle F. Philhower, III 2020-12-12 18:59 ` Brian Inglis 2020-12-14 9:30 ` Corinna Vinschen 0 siblings, 2 replies; 8+ messages in thread From: Earle F. Philhower, III @ 2020-12-12 17:56 UTC (permalink / raw) To: newlib > On 2020-11-16 18:43, C Howland via Newlib wrote:>> On Monday, November 16, 2020 5:30 PM, Brian Inglis wrote: >>> On 2020-11-16 08:13, Corinna Vinschen via Newlib wrote: >>>> On Nov 14 15:03, Earle F. Philhower, III via Newlib wrote: >>>>> Attached is a patch which extends the tzset() function to support a >>>>> format for "unnamed" TZ environment timezones which use "<+/-nn>" as >>>>> the timezone name instead of an alphabetic name. These are supported >>>>> in glibc and are present in several major TZ databases that we use on >>>>> the ESP8266 Arduino core. For example, ...>>>> Basically this looks ok. I have two nits, though.>>>> - Now that the scanning got more complicated than a single sscanf call, >>>> this crys out for a helper function doing the actual scanning for >>>> both, std and dst strings. This could be an inline function which is >>>> only inlined >>>> #if !defined(PREFER_SIZE_OVER_SPEED) && !defined(__OPTIMIZE_SIZE__)>>>> - The strcat call seems a bit heavy. What about sth like this instead: Thanks Corinna. I've factored out the common name scanner to its own static function, removed the strcat in favor of your suggestion, and added documentation in the code about its function. However, I'm not sure it makes sense to worry about size/speed optimizations here. This is tzset_r and the normal case is to call it ~1 time per application startup to set the localtime offsets using the TZ environment variable, and never again. >>> Should consider modifying the PD TZ project tzcode reference src >>> localtime.c tzparse() >>> https://github.com/eggert/tz/blob/master/localtime.c#L1069 ... Brian, thanks for the suggestion but I'd be very worried about opening up this function so completely. There's unfortunately not a set of unit tests for the time routines in newlib, and there's not any particular issue that's seen other than this timezone naming scheme. Updated patch follows, I hope it addresses the issues above.Anything missing, please do let me know. ----Extends the tzset() function to support <quoted> TZ environment timezones names. For example, "<+01>-1" is the timezone environment for TZ_Africa_Casablanca Signed-off-by: Earle F. Philhower, III <earlephilhower@yahoo.com> --- newlib/libc/time/tzset_r.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/newlib/libc/time/tzset_r.c b/newlib/libc/time/tzset_r.c index 9e0cf834b..61228ab72 100644 --- a/newlib/libc/time/tzset_r.c +++ b/newlib/libc/time/tzset_r.c @@ -13,6 +13,30 @@ static char __tzname_std[11]; static char __tzname_dst[11]; static char *prev_tzenv = NULL; +/* Handle parsing of both unquoted and <quoted> forms of the timzeone name. */ +/* Returns 0 on failure, 1 on successful parsing, placing name and count in */ +/* passed-in __tzname, n variables. */ +static int +__parse_tzname_string(const char *tzenv, char *__tzname, int *n) +{ + if (tzenv[0] == '<') + { + /* This is of the quoted form "<[+-]nn>" so needs a different parsing */ + if (sscanf (tzenv, "%9[^>]>%n", __tzname, n) <= 0) + return 0; + /* Include the final > in the timezone name */ + __tzname[*n - 1] = '>'; + __tzname[*n] = 0; + } + else + { + /* Unquoted timezone format parser */ + if (sscanf (tzenv, "%10[^0-9,+-]%n", __tzname, n) <= 0) + return 0; + } + return 1; +} + void _tzset_unlocked_r (struct _reent *reent_ptr) { @@ -45,7 +69,7 @@ _tzset_unlocked_r (struct _reent *reent_ptr) if (*tzenv == ':') ++tzenv; - if (sscanf (tzenv, "%10[^0-9,+-]%n", __tzname_std, &n) <= 0) + if (!__parse_tzname_string (tzenv, __tzname_std, &n)) return; tzenv += n; @@ -69,7 +93,7 @@ _tzset_unlocked_r (struct _reent *reent_ptr) _tzname[0] = __tzname_std; tzenv += n; - if (sscanf (tzenv, "%10[^0-9,+-]%n", __tzname_dst, &n) <= 0) + if (!__parse_tzname_string (tzenv, __tzname_dst, &n)) { /* No dst */ _tzname[1] = _tzname[0]; _timezone = tz->__tzrule[0].offset; -- 2.17.1 Thanks,-EFP3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Add support for TZ names with <> in tzset 2020-12-12 17:56 ` Earle F. Philhower, III @ 2020-12-12 18:59 ` Brian Inglis 2020-12-14 9:30 ` Corinna Vinschen 1 sibling, 0 replies; 8+ messages in thread From: Brian Inglis @ 2020-12-12 18:59 UTC (permalink / raw) To: newlib On 2020-12-12 10:56, Earle F. Philhower, III via Newlib wrote: > On 2020-11-16 18:43, C Howland via Newlib wrote: >> On Monday, November 16, 2020 5:30 PM, Brian Inglis wrote: >>>> On 2020-11-16 08:13, Corinna Vinschen via Newlib wrote: >>>>> On Nov 14 15:03, Earle F. Philhower, III via Newlib wrote: >>>> Should consider modifying the PD TZ project tzcode reference src >>>> localtime.c tzparse() >>>> https://github.com/eggert/tz/blob/master/localtime.c#L1069 > thanks for the suggestion but I'd be very worried about opening > up this function so completely. There's unfortunately not a set of unit > tests for the time routines in newlib, and there's not any particular > issue that's seen other than this timezone naming scheme. The deltas between newlib, BSD, and tzcode are mainly minor feature checks, and functions dropped from tzcode due to being non-functional, outdated, or rarely used, retained in libc for ABI compatibility. -- Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada This email may be disturbing to some readers as it contains too much technical detail. Reader discretion is advised. [Data in binary units and prefixes, physical quantities in SI.] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Add support for TZ names with <> in tzset 2020-12-12 17:56 ` Earle F. Philhower, III 2020-12-12 18:59 ` Brian Inglis @ 2020-12-14 9:30 ` Corinna Vinschen 1 sibling, 0 replies; 8+ messages in thread From: Corinna Vinschen @ 2020-12-14 9:30 UTC (permalink / raw) To: Earle F. Philhower, III; +Cc: newlib Hi Earle, On Dec 12 17:56, Earle F. Philhower, III via Newlib wrote: > > > On 2020-11-16 18:43, C Howland via Newlib wrote:>> On Monday, November 16, 2020 5:30 PM, Brian Inglis wrote: > >>> On 2020-11-16 08:13, Corinna Vinschen via Newlib wrote: > >>>> On Nov 14 15:03, Earle F. Philhower, III via Newlib wrote: > >>>>> Attached is a patch which extends the tzset() function to support a > >>>>> format for "unnamed" TZ environment timezones which use "<+/-nn>" as > >>>>> the timezone name instead of an alphabetic name. These are supported > >>>>> in glibc and are present in several major TZ databases that we use on > >>>>> the ESP8266 Arduino core. For example, > ...>>>> Basically this looks ok. I have two nits, though.>>>> - Now that the scanning got more complicated than a single sscanf call, > >>>> this crys out for a helper function doing the actual scanning for > >>>> both, std and dst strings. This could be an inline function which is > >>>> only inlined > >>>> #if !defined(PREFER_SIZE_OVER_SPEED) && !defined(__OPTIMIZE_SIZE__)>>>> - The strcat call seems a bit heavy. What about sth like this instead: > > Thanks Corinna. I've factored out the common name scanner to its own > static function, removed the strcat in favor of your suggestion, and > added documentation in the code about its function. > > However, I'm not sure it makes sense to worry about size/speed > optimizations here. This is tzset_r and the normal case is to call > it ~1 time per application startup to set the localtime offsets > using the TZ environment variable, and never again. The size optimization would be the point here in the first place, the speed optimization by inlining secondary. However, adding the aforementioned #if is really not much of a problem to handle both preferences alike. Other than that, the patch looks ok. Can you please send it in `git format-patch' format when you're ready? Thanks, Corinna ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-12-14 9:30 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <080401d6bada$6c52f060$44f8d120$.ref@yahoo.com> 2020-11-14 23:03 ` [PATCH] Add support for TZ names with <> in tzset earlephilhower 2020-11-16 15:13 ` Corinna Vinschen 2020-11-16 22:30 ` Brian Inglis [not found] ` <DM2P110MB01699F833CC488133A5AE9E99AE20@DM2P110MB0169.NAMP110.PROD.OUTLOOK.COM> 2020-11-17 1:43 ` Fw: " C Howland 2020-11-17 4:59 ` Brian Inglis 2020-12-12 17:56 ` Earle F. Philhower, III 2020-12-12 18:59 ` Brian Inglis 2020-12-14 9:30 ` Corinna Vinschen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).