Re: [PATCH v5 1/2] Y2038: Add 64-bit time for all architectures

[Albert ARIBAUD <albert dot aribaud at 3adev dot fr>]

Hi Paul,

On Tue, 19 Jun 2018 16:25:41 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 06/18/2018 12:14 PM, Albert ARIBAUD (3ADEV) wrote:
> > +/* Check whether a time64_t value fits in a time_t.  */
> > +static inline bool
> > +fits_in_time_t (__time64_t t)
> > +{
> > +  return t == (time_t) t;
> > +}  
> 
> This static function is used nowhere in this patch series. Shouldn't its 
> introduction be delayed to the first patch that actually needs it?
> 
> Also, looking at the two future uses of this function, they're both of 
> the form:
> 
>    __time64_t t64 = [something];
>    if (fits_in_time_t (t64))
>      return (time_t) t64;
>    __set_errno (EOVERFLOW);
>    return -1;
> 
> Wouldn't it be better to have these uses do the following instead? This 
> would be just as clear, and would avoid the need for casts and for the 
> fits_in_time_t function.
> 
>    __time64_t t64 = [something];
>    time_t t = t64;
>    if (t == t64)
>      return t;
>    __set_errno (EOVERFLOW);
>    return -1;

I can defer the function definition to within the second patch in the
series.

Regarding the cast, there is no way to reduce the /need/ for casts, as
we /do/ need one here. What we can do is reduce the number of explicit
casts, as in the example above.

But I disagree that the resulting code would be as clear as the one in
the patch: it would in fact be less clear, because the intent of the
code would become implicit rather than explicit in two places:

* replacing the call to (and consequently the definition of) function
  fits_in_time_t() with an in-line equality test obscures the reason
  why we do this test;
* hiding the cast itself makes it harder to see that this cast is
  the important thing happening here.

The goal of the code is to perform the cast, so it is best if said
cast is explicit. Here, the end result of obscuring the function hiding
an important cast would not be worth the added line and variable in
every place where we need reduction to time_t.

But we can do something that keeps the code explicit /and/ reduces
it properly, based on the fact that we always call fits_in_time_t() to
perform a cast to time_t or set errno and reduce to -1 if we cannot
cast.

We could put the consequence with the test, and instead of defining
fits_in_time_t(), we could define reduce_to_time_t() as follows:

	/* Check whether a time64_t value fits in a time_t.  */
	static inline time_t
	reduce_to_time_t (__time64_t t)
	{
	  if (t == (time_t) t)
	    return t;
	  __set_errno (EOVERFLOW);
	  return -1;
	}

Then wherever we would write

	time_t some_func (...)
	{
	  ...
	  if (fits_in_time_t (t64))
	    return (time_t) t64;
	  __set_errno (EOVERFLOW);
	  return -1;
	}

.... we could now write

	time_t some_func (...)
	{
	  ...
	  return reduce_to_time_t (t64);
	}

... which would be as explicit (both in the definition and in the calls)
and take up much less source code.

Cordialement,
Albert ARIBAUD
3ADEV