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

[Pierre Neidhardt]

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".

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:

	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.

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.

--
Pierre Neidhardt

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