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

[Dave Reisner]

On Tue, Oct 20, 2015 at 10:29:43AM +0200, Pierre Neidhardt wrote:
> On 15-10-19 17:22:55, Dave Reisner wrote:
> > On Sat, Oct 17, 2015 at 07:02:57PM +0200, Pierre Neidhardt wrote:
> > > +/* we truncate titles longer than this arbitrary value */
> > 
> > Well no, it isn't arbitrary, you said so yourself. Why not state this?
> > Something like:
> > 
> > "As of such and such a date, the longest relevant string from the PO
> > files was measured to be X characters long. We set this to 50 to allow
> > for some potential growth."
> > 
> > > +#define TITLE_MAXLEN 50
> > > +
> > > +/* A title is referenced by titles[T_XXX]. The T_ prefix avoids shadowing. */
> > 
> > Not a useful comment. Nowhere in this file do you explain what a "title"
> > is.
> 
> The term "title" is used (not only by me) here and there throughout the code.
> (See 'util.c'.) I thought this was a given, but it does not cost much to explain
> here what it is. What is more useful though is to explain what an "aligned
> title" is.

Ack.

> 
> > The fact that you use a prefix is quite obviously done to make for
> > unique enum identifiers.
> > 
> > > +enum {
> > > +	T_ARCHITECTURE = 0,
> > > +	T_BACKUP_FILES,
> > > +	T_BUILD_DATE,
> > > +	T_COMPRESSED_SIZE,
> > > +	T_CONFLICTS_WITH,
> > > +	T_DEPENDS_ON,
> > > +	T_DESCRIPTION,
> > > +	T_DOWNLOAD_SIZE,
> > > +	T_GROUPS,
> > > +	T_INSTALL_DATE,
> > > +	T_INSTALL_REASON,
> > > +	T_INSTALL_SCRIPT,
> > > +	T_INSTALLED_SIZE,
> > > +	T_LICENSES,
> > > +	T_MD5_SUM,
> > > +	T_NAME,
> > > +	T_OPTIONAL_DEPS,
> > > +	T_OPTIONAL_FOR,
> > > +	T_PACKAGER,
> > > +	T_PROVIDES,
> > > +	T_REPLACES,
> > > +	T_REPOSITORY,
> > > +	T_REQUIRED_BY,
> > > +	T_SHA_256_SUM,
> > > +	T_SIGNATURES,
> > > +	T_URL,
> > > +	T_VALIDATED_BY,
> > > +	T_VERSION,
> > > +	TITLE_COUNT
> > 
> > Please comment that this is a sentinel value and needs to be last...
> 
> OK.
> 
> > 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.

> > > +};
> > > +
> > > +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.

> > > +
> > > +/** Truncate all titles longer than TITLE_MAX and pad the rest with spaces so
> > > + * that they align with the longest title. " :" is appended to all titles.
> > > + *
> > > + * Truncation allows us to avoid allocations and clean up code.
> >
> > This isn't a useful comment. It's stack allocation which avoids the
> > cleanup and allocation, not truncation. Being forced to truncate is a
> > side effect of using stack allocating.
> 
> Agreed.
> 
> > Perhaps something like:
> >
> > "Build an array of localized strings used to create aligned titles in
> > verbose package information. Storage for strings is stack allocated and
> > naively truncated to TITLE_MAXLEN characters."
> 
> Your version is a bit misleading: we could think that the alignment happens
> outside this function. It misses an explanation for what "aligning" titles
> precisely means. I'd say:
> 
> > Build the `titles` array of localized titles and pad them with spaces so
> > that they align with the longest title. Storage for strings is stack
> > allocated and naively truncated to TITLE_MAXLEN characters."

Sounds good.

> 
> -- 
> Pierre Neidhardt
> 
> It is better to have loved and lost than just to have lost.