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
variablesand 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_POINTinstead. 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
orlarger 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
