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
format-width-20130228.patch.bz2
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
