Hi Paul,

> On 2/27/19 3:20 AM, Lukasz Majewski wrote:
> > +/* Another name for `__mktime64'.  */
> > +extern __time64_t __timelocal64 (struct tm *__tp) __THROW;  
> 
> In hindsight the name 'timelocal' was a mistake: it's not a portable
> name and its use has not caught on. Although we need to keep
> 'timelocal' for backwards compatibility, there's no need to define
> 'timelocal64', as the very few people who need such a function can
> just call mktime64. So I suggest removing all traces of timelocal64,
> __timelocal64, etc. from the patch.
> 
> 
> > +/* Check whether a time64_t value fits in a time_t.  */
> > +static inline bool
> > +fits_in_time_t (__time64_t t64)
> > +{
> > +  time_t t = t64;
> > +  return t == t64;
> > +}
> > +  
> 
> This function is used only in time/mktime.c, and so should be defined
> there. This will help sharing with Gnulib, which doesn't have
> include/time.h.

Some patches on top of this one (y2038 related) use fits_in_time_t() to
check if 64 bit wrapper on 32 bit version of function (like
__clock_gettime()) shall proceed or return EOVERFLOW.

The question is if there is a more suitable place than include/time.h
header to have fits_in_time() reused by both Glibc and Gnulib?

> 
> The function's name should not end with "_t" since that's in the
> reserved namespace (important for Gnulib).

Ok.

> 
> 
> > -verify (TYPE_IS_INTEGER (time_t));
> > +verify (TYPE_IS_INTEGER (__time64_t));  
> 
> Please remove this line instead. It dates back to old POSIX, which
> allowed time_t to be a floating-point type. POSIX no longer allows
> this and we needn't worry about that possibility any more.
> 
> 
> > +  __time64_t t64;
> > +  time_t t;
> > +  struct tm tp0 = *tp;  
> There is no need for both t and t64. Just declare "__time64_t t;" and
> replace all uses of t64 with t. Also, please consistently rename tp0
> to tm0, since it's not a pointer.
> 
> 
> > +  t = t64;
> > +  if (t != t64)  
> 
> Replace this with "if (! fits_in_time_t (t))" (but rename the
> function).
> 
> 
> > +/* The 32-bit-time wrapper.  */
> > +time_t
> > +mktime (struct tm *tp)
> > +{
> > +  __time64_t t64 = __mktime64 (tp);
> > +  if (fits_in_time_t (t64))
> > +    return t64;
> > +  __set_errno (EOVERFLOW);
> > +  return -1;
> > +}  
> 
> Fix this so that it doesn not modify *TP when failing due to
> EOVERFLOW. The manual says mktime doesn't change *TP on failure.
> 
> 
> Similar comments apply to the implementation of timegm.
> 

Thanks for your comments.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de