Re: Re: proposal: a width specification for s specifier (format function), fix behave when positional and ordered placeholders are used

Hello, Could you let me review this patch?

> >> * merged Dean's doc
> >> * allow NULL as width

I understand that this patch aims pure expansion of format's
current behavior and to mimic the printf in SUS glibc (*1).

(*1) http://pubs.opengroup.org/onlinepubs/009695399/functions/printf.html

This patch seems to preserve the behaviors of current
implement. And also succeeds in mimicking almost of SUS without
very subtle difference.

Attached is the new patch which I've edited following the
comments below and some fixed of typos, and added a few regtests.

If you have no problem with this, I'll send this to committer.

What do you think of this?

My comments are below,

======================================
Following is a comment about the behavior.

- The minus('-') is a flag, not a sign nor a operator. So this
seems permitted to appear more than one time. For example,
printf(">>%-------10s<<", "hoge") yields the output
">>hoge______<<" safely. This is consistent with the behavior
when negative value is supplied to '-*' conversion.

Followings are some comments about coding,

in text_format_parse_digits,

- is_valid seems to be the primary return value so returning
this as function's return value should make the caller more
simple.

- Although the compiler should deal properly with that, I don't
think it proper to use the memory pointed by function
parameters as local working storage. *inum and *is_valid in
the while loop should be replaced with local variables and
set them after the values are settled.

for TEXT_FORMAT_NEXT_CHAR,

- This macro name sounds somewhat confusing and this could be
used also in text_format_parse_digits. I propose
FORWARD_PARSE_POINT instead. Also I removed end_ptr from
macro parameters but I'm not sure of the pertinence of that.

for text_format_parse_format:

- Using start_ptr as a working pointer makes the name
inappropriate.

- Out parameters seems somewhat redundant. indirect_width and
indirect_width_parameter could be merged using 0 to indicate
unnumbered.

for text_format:

- maximum number of function argument limited to FUNC_MAX_ARGS
(100), so no need to care of wrap around of argument index, I
suppose.

- Something seems confusing at the lines follow

| /* Not enough arguments? Deduct 1 to avoid counting format string. */
| if (arg > nargs - 1)

This expression does not have so special meaning. The maximum
index in an zero-based array should not be equal to or larger
than the number of the elements of it. If that's not your
intent, some rewrite would be needed..

- Only int4 is directly read for width value in the latest
patch, but int2 can also be directly readable and it should
be needed.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center