Re: [PATCH] Refactor mktime and add the POSIX function timegm

On 2021-05-03 02:37, R. Diez via Newlib wrote:
> I have manually reconstructed the patch from Andrew Russell posted here:
> 
> https://sourceware.org/legacy-ml/newlib/2018/msg00824.html
> 
> Is the new implementation of __is_leap_year() right? I would rather keep the old ISLEAP implementation.
> 
> R. Diez (1):
>    Refactor mktime and add the POSIX function timegm
> 
>   newlib/libc/include/time.h   |  3 ++
>   newlib/libc/saber            |  1 +
>   newlib/libc/time/Makefile.am |  2 +
>   newlib/libc/time/Makefile.in |  9 +++++
>   newlib/libc/time/local.h     |  2 +
>   newlib/libc/time/mktime.c    | 78 +++++++++++++++++++++++++-----------
>   newlib/libc/time/timegm.c    | 63 +++++++++++++++++++++++++++++
>   7 files changed, 135 insertions(+), 23 deletions(-)
>   create mode 100644 newlib/libc/time/timegm.c

 >+<<timegm>> is a nonstandard GNU extension to POSIX also present on BSD.

Don't see where *non-inline* static functions and trivial renames are 
improvements over macros.
You removed a cast that may have been added to avoid overflow, probably in 32 
bit implementations, which is a continuing problem in these functions.
It may be unnecessary to add a new file for a small new function.

For patches of this sort, you normally want to avoid trivial changes so the 
substance of the patch stands out.

It may have been better to compare against the upstream BSD (or tzcode) sources 
and include one of those implementations of timegm, with any required changes, 
rather than refactor including trivial changes, making it difficult to rebase 
against the upstream and merge changes in future.

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