[pacman-dev] [PATCH v2 3/3] Align titles automatically in information display

[Dave Reisner]

On Tue, Oct 20, 2015 at 02:11:41PM +0200, Pierre Neidhardt wrote:
> On 15-10-20 06:58:58, Dave Reisner wrote:
> > On Tue, Oct 20, 2015 at 10:29:43AM +0200, Pierre Neidhardt wrote:
> > > On 15-10-19 17:22:55, Dave Reisner wrote:
> > > > I'd also love to claim that this needs to share the same prefix as the enums
> > > > it belongs to (and have an underscore prefix), but there's not really any
> > > > prevailing coding style in ALPM that I can lean on to "enforce" that.
> > >
> > > Agreed, but I prefer keeping underscore prefixes for library code. This is not
> > > ALPM but the pacman frontend if this is what you meant.
> > >
> >
> > I've no idea why you would suddenly change coding style for internal
> > library code versus frontend code.
> 
> I believe there is a misunderstanding here. To me, underscore prefixed variables
> are used in libraries as a mean to tell the calling programs "don't use me, I'm
> internal".

That sounds like bad API design. If you didn't want it exposed and used,
you shouldn't have exposed it. The underscore prefix here is just a case
of "one of these things is not like the other". It belongs, because it's
related to the rest of the enum, but it isn't one of the identifiers
used as part of the enum to index a title string.

> My patch only changes 'package.c' which is part of the frontend code. Following
> the aforementioned coding style, I would rather not prefix any variable with an
> underscore.
> 
> That is really a nit though, if you really like it better, I don't mind
> changing it.
> 
> > > > > +};
> > > > > +
> > > > > +static char titles[TITLE_COUNT][TITLE_MAXLEN * sizeof(wchar_t)];
> >
> > I do wonder if it isn't best to also only use _T_MAX here and elsewhere
> > in the code where you traverse the arrays use:
> >
> >   sizeof(titles)/sizeof(titles[0])
> >
> > And perhaps introduces an ARRAYSIZE or ELEMENTSOF macro to do this for
> > us (we'd make use it elsewhere in the lib and frontend). This is safer
> > and clearer to the reader that you're traversing the precise bounds of
> > the array, not some max index that you "hope" lines up with the bound.
> 
> Agreed, plus this is idiomatic enough in C. But! From HACKING:
> 
> > 6.  The sizeof() operator should accept a type, not a value. (TODO: in certain
> >     cases, it may be better- should this be a set guideline? Read "The Practice
> >     of Programming")
> 
> I must say I agree with the TODO. Shall we update this? From your first review,
> you asked for 2 changes that go againt paragraph 6:

Seems fine to me. To be explicit, the referenced literature says "Use
the language to calculate the size of an object" and gives an example
macro of:

  #define NELEMS(array) (sizeof(array) / sizeof(array[0]))

This is pragmatic and, more importantly, safe. I'm not sure why you'd
want to prefer a type over a value. The former seems to me to be more
error prone than using the value.

> 	static const size_t title_suffix_len = sizeof(title_suffix);
> 
> and
> 
> 	wcstombs(titles[i], wbuf[i], sizeof(wbuf[i]));
> 
> Besides, I have seen a few other places in the current code where §6 is being
> transgressed.

Sounds likely...

> Should we agree on this, shall we place the ARRAYSIZE macro in util-common.h?
> Other typical names include LENGTH, SIZE, COUNT_OF, ELEMENTSOF. I like ARRAYSIZE
> better as it makes clear it must be an array.

Please do. We can do cleanup in other places in another patch, but it'll
be nice to have it here.

> 
> --
> Pierre Neidhardt
> 
> An optimist is a guy that has never had much experience.
> 		-- Don Marquis