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

[Joseph Myers <joseph at codesourcery dot com>]

On Tue, 18 Sep 2018, Albert ARIBAUD (3ADEV) wrote:

> * Add macro __TIMESIZE equal to the bit size of time_t.
>   It equals the architecture __WORDSIZE except for x32
>   where it equals 64.
> * Add type __time64_t which is always 64-bit. On 64-bit
>   architectures and on x32, it is #defined as time_t.
>   On other architectures, it has its own definition.
> * Replace all occurrences of internal_time_t with
>   __time64_t.

These comments are primarily on the proposed commit message, not the code 
changes, although they do have implications for comments in the code.

First: I am assuming this patch and commit message are proposed for commit 
as-is, since it isn't marked as an RFC; if something is known not to be 
ready for commit as-is then it's best to mark it as an RFC and be clear 
about the things you know are not yet ready.

We've agreed that commit messages need to be reviewed just as much as code 
changes.  They are the primary piece of information about a change people 
will be looking at when looking at that change in future, so they need to 
be written to make sense to those unknown future readers, who may not have 
the context we have now - as well as providing an explanation to the 
reviewers now.

A commit message generally needs to include both (a) the human-level 
explanation of what the change does and why, and (b) the proposed 
ChangeLog entry.  (a) would normally be some number of paragraphs, 
depending on the nature and complexity of the change; it's only for some 
very simple changes that the Subject: line of the patch submission is 
sufficient without further paragraphs.

In this patch submission, you're missing the ChangeLog entry,  In some 
others, you have it first, which is confusing to readers; the ChangeLog 
entry is expected to be the last thing before the patch itself, so if it's 
first that gives the impression there is no human-level explanation at 
all.

"bit size of time_t" is ambiguous.  Do you mean the size of time_t *for 
the default ABI for this libc*, or the size for the current compilation?  
Once _TIME_BITS=64 is supported, those can differ.  And since this patch 
is part of preparing for supporting _TIME_BITS=64, it's critical to know 
which is meant, so that __TIMESIZE conditionals in this or subsequent 
patches can be properly reviewed according to whether they would be 
correct in the context of _TIME_BITS=64.  I'm guessing that you mean for 
the default ABI for this libc (so it becomes a suitable condition for 
whether the default time_t ABIs are aliases to the 64-bit ones or wrappers 
round them), but that needs to be explicit, both in the commit message and 
in the comment on the default definition of this macro.

Then, I'd expect some high-level description of the purpose of the patch 
before discussing anything about the individual macros / types added.  
That is, something along the lines of:

  glibc support for 64-bit time_t on 32-bit architectures will involve 
  primarily using 64-bit times inside glibc, with conversions to and from 
  32-bit times taking place as necessary for interfaces using such times.  
  This requires a glibc-internal name for a type for times that are always 
  64-bit.  To determine whether the default time_t interfaces are 32-bit 
  and so need conversions, or are 64-bit and so are compatible with the 
  internal 64-bit type without conversions, a macro giving the size of the 
  default time_t is also required.  Given the new type, it can then 
  replace uses of internal_time_t.

After that, the descriptions of the individual macros / types added could 
be given (whether as paragraphs or as bullet points).

> +#ifndef _BITS_TYPES_H
> +# error "Never include <bits/time64.h> directly; use <sys/types.h> instead."
> +#endif
> +
> +#ifndef	_BITS_TIME64_H
> +#define	_BITS_TIME64_H	1
> +
> +/* See <bits/types.h> for the meaning of these macros.  This file exists so
> +   that <bits/types.h> need not vary across different GNU platforms.  */

I don't think this comment (copied from bits/typesizes.h) is applicable 
here as-is.  It's one macro not multiple macros, and that reason is the 
reason for bits/typesizes.h to exist, not so much the reason for 
bits/time64.h to exist.  When it's just one macro, you may as well give 
the meaning directly rather than pointing to another file for it.

> +/* Size in bits of the 'time_t' type.  */
> +#define __TIMESIZE	__WORDSIZE

This is what needs clarifying as being about the default ABI not the 
current compilation.

> +#if __TIMESIZE == 64
> +# define __time64_t __time_t
> +#else
> +__STD_TYPE __TIME64_T_TYPE __time64_t;	/* Seconds since the Epoch (_TIME_BITS==64).  */

I don't think the reference to _TIME_BITS==64 is helpful at this point; 
right now, this is the internal type for times (which would also be used 
for time_t when _TIME_BITS=64).

> +#ifndef	_BITS_TIME64_H
> +#define	_BITS_TIME64_H	1
> +
> +/* See <bits/types.h> for the meaning of these macros.  This file exists so
> +   that <bits/types.h> need not vary across different GNU platforms.  */

Another repeat of the same comment that I don't think is helpful.

-- 
Joseph S. Myers
joseph@codesourcery.com