Hello I have no objections,
Thank you for update Regards Pavel 2013/2/28 Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.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 > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
