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 Wed, 20 Jun 2018 09:01:08 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> Albert ARIBAUD wrote:
> 
> > 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  
> 
> A terminology point: the C Standard uses the word "cast" to describe what you're 
> calling an "explicit cast", and it uses the word "conversion" to describe what 
> you're calling a "cast". Let's stick to the standard terminology as it makes for 
> less confusion.

Agreed.

> > 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:  
> 
> First, in C, casts are more dangerous than other conversions because they allow 
> more typos to go unchecked. If you mistakenly cast an integer to a pointer, the 
> compiler often does not complain, but if you mistakenly convert an integer to a 
> pointer without casting it, the compiler will catch your mistake and report an 
> error. For this reason, C casts should not be used unless necessary (and they 
> are not necessary here).

Indeed when the cast is /created/, there is a risk that it be wrong.
But once it is created and reviewed and found correct, then in a
function that risk is gone for good IMO since both the type being cast
to and the type being cast from are known -- of course, I would stand
firm against any cast being done to a macro argument, as you cannot
tell what type it might have, if it has any.

> Second, the code I proposed is completely obvious. One cannot read code like this:
> 
>     type1 x = ...;
>     type2 y = x;
>     if (y == x) ...
> 
> without immediately knowing what's going on.

Here, I would beg to differ on what one can or cannot immediately know
what's going on without explicit clues. In my experience, what one
immediately knows from a piece of code varies wildly across
individuals, and Murphy is extremely efficient in ensuring that what
goes without saying always ends up going wrong at some point.

> Third, as you noted, the proposed fits_in_time_t function does a poor job of 
> moving common code into a single function. To do a better job we would need 
> something like the reduce_to_time_t function of your email. Unfortunately, as 
> you noted in a later email, reduce_to_time_t doesn't work because of include 
> problems. The exact same thought process went through my head when I wrote my 
> review. That is, I thought "fits_in_time_t is a bad helper function, and trying 
> to improve it by having a helper function that captures the actual idea won't 
> work due to include hassles, so let's just do things directly; it's just as 
> clear and it solves the problem better".
> 
> So, let's just do things directly; it's just as clear and it solves the problem 
> better.

There's the possibility of replacing function reduce_to_time_t() with a
macro, which would postpone evaluation of __set_errno() until within
the calling C file, but then, a cast in a macro is a worse idea than a
cast in a function.

I still dislike the idea of duplicating code, but here specifically, I
can't see another way without doing bigger and deeper changes than I
need to. I also still don't think the cast discussed here would be a
risk or bad practice, but I'm ok with removing it provided the readers
get a stronger hint on what's going on unsaid. So I'll go with the
code you suggest, with a more explicit name for the temporary and a
one-line short comment.

   /* Convert from __time64 to time_t or fail with EOVERFLOW.  */
   time_t t32 = t64;
   if (t32 == t64)
     return t32;
   __set_errno (EOVERFLOW);
   return -1;

Cordialement,
Albert ARIBAUD
3ADEV