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
