Re: pic anomalies

[Ingo Schwarze]

Hi Colin and Larry,

Colin Watson wrote on Sun, Dec 29, 2019 at 11:17:26PM +0000:

> LGTM as far as it goes.

thanks to both of you for checking, i have put it in.

> If I were doing it I think I'd take the opportunity to simplify it
> a little further into this sort of structure:

As i said, i dislike mixing an outright bugfix with substantial
refactoring.  When people look at the history later on, that would
make it harder to understand what was going on.

>   if (*form == '%') {
>     form++;
>     result += '%';
>   }
>   else {
>     ...
>     snprintf(sprintf_buf, sizeof(sprintf_buf),
>              one_format.contents(), v[i++]);
>     result += sprintf_buf;
>   }
>   one_format.clear();
> 
> But I understand if you'd prefer not to do that here.

You are very welcome to do more cleanup.  Lacking extensive experience
with C++ (i'm doing more C programming), i'm not so sure i should
attempt major cleanup in such code.  But if you do it, i might
do testing and review.

Here are aspects that it seems to me could be improved:

 * line 1896, while (*form):
   Testing char as if it were boolean seems dubious style to me;
   "while (*form != '\0')" would seem clearer because it makes
   the data type obvious on first sight.
 * lines 1897 and 1926/27:
   I find code hard to read when the consequences of a test are
   far away from the test itself.  Doing
        if (*form != '%') {
                result += *form++;
                continue;
        }
   up front would seem much easier to read because it leaves
   the rest of the body to the case of a conversion specification
   and because it gets the simpler case out of the way quickly.
 * line 1899:
   I think the "*form == '%'" test should go *before*
   the width.precision for loop.  While some stdio implementations
   accept stuff like "%42%", the original C standard says it is
   unspecified, so for pic(1) to be portable, i think it should
   be rejected.  It could for example take a form like:
        if (*++form == '%') {
                form++;
                result += '%';
                continue;
        }
        one_format += '%';
   again handling the trivial case up front and leaving the rest
   of the loop to the substantial processing.
   Also, this way, one_format only needs to be initialized when
   it is actually needed, which seems clearer.
 * Then the two error handling ifs, both with "break;",
   and finally the one_format snprintf.

That way, the maximum indentation would also be reduced to thee
levels (function, while, and ifs) while right now it has five levels.

Yours,
  Ingo