Hi Torsten,

On Mon, 3 Apr 2017, Torsten Bögershausen wrote:

> On 02/04/17 21:06, Johannes Schindelin wrote:
> > In its `atom_value` struct, the ref-filter source code wants to store
> > different values in a field called `ul` (for `unsigned long`), e.g.
> > timestamps.
> >
> > However, as we are about to switch the data type of timestamps away from
> > `unsigned long` (because it may be 32-bit even when `time_t` is 64-bit),
> > that data type is not large enough.
> >
> > Simply change that field to use `uintmax_t` instead.
> >
> > This patch is a bit larger than the mere change of the data type
> > because the field's name was tied to its data type, which has been fixed
> > at the same time.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  ref-filter.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/ref-filter.c b/ref-filter.c
> > index 9c82b5b9d63..8538328fc7f 100644
> > --- a/ref-filter.c
> > +++ b/ref-filter.c
> > @@ -351,7 +351,7 @@ struct ref_formatting_state {
> >  struct atom_value {
> >   const char *s;
> >   void (*handler)(struct atom_value *atomv, struct ref_formatting_state
> >   *state);
> > -	unsigned long ul; /* used for sorting when not FIELD_STR */
> > +	uintmax_t value; /* used for sorting when not FIELD_STR */
> >  	struct used_atom *atom;
> >  };
> >
> > @@ -723,7 +723,7 @@ static void grab_common_values(struct atom_value *val,
> > int deref, struct object
> >    if (!strcmp(name, "objecttype"))
> >    	v->s = typename(obj->type);
> > 		else if (!strcmp(name, "objectsize")) {
> > -			v->ul = sz;
> > +			v->value = sz;
> >    	v->s = xstrfmt("%lu", sz);
> >    }
> >    else if (deref)
> > @@ -770,8 +770,8 @@ static void grab_commit_values(struct atom_value *val,
> > int deref, struct object
> >    	v->s = xstrdup(oid_to_hex(&commit->tree->object.oid));
> >    }
> >    else if (!strcmp(name, "numparent")) {
> > -			v->ul = commit_list_count(commit->parents);
> > -			v->s = xstrfmt("%lu", v->ul);
> > +			v->value = commit_list_count(commit->parents);
> > +			v->s = xstrfmt("%lu", (unsigned long)v->value);
> 
> If we want to get rid of "%lu" at some day, we can do like this:
> v->s = xstrfmt("%" PRIuMAX, v->value);
> Or, to make clear that under all circumstances an unsigned long is big enough
> to
> hold the counter, for readers in the future, use something like this:
> 			v->s = xstrfmt("%lu", (xulong_t)v->value);

We could do that, yes.

But part of my patch series is to clarify in a semantic way what the
purpose of the code is. Your solution would keep it syntactically correct,
of course, but it would also make it semantically unclear again.

By writing "%"PRIutime *instead of* "%"PRIuMAX, we are saying: look, we
are talking about a timestamp here. That would not at all be clear if we
wrote "%"PRIuMAX.

Ciao,
Johannes