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

Hello

I have no objections,

Thank you for update

Regards

Pavel

2013/2/28 Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>:
> 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
>