Thanks. I did a quick review of this patch, and I think it's almost RFC.
Thanks for reviewing.
- In func.sgml, it seems we've lost this bit:
<para> <literal>TM</literal> does not include trailing blanks. <function>to_timestamp</function> and <function>to_date</function> ignore the <literal>TM</literal> modifier. </para>
Does that mean the function no longer ignore the TM modifier? That would be somewhat problematic (i.e. it might break some user code). But looking at the code I don't see why the patch would have this effect, so I suppose it's merely a doc bug.
It is intentional. This patch uses the TM modifier to identify the usage of localized names as input for to_timestamp() and to_date().
- I don't think we need to include examples how to_timestmap ignores case, I'd say just stating the fact is clear enough. But if we want to have examples, I think we should not inline in the para but use the established pattern:
<para> Some examples: <programlisting> ... </programlisting> </para>
which is used elsewhere in the func.sgml file.
I was trying to match the style surrounding the usage notes for date/time formatting [1]. Agreed that it is not worth an example on its own, so dropped.
- In formatting.c the "common/unicode_norm.h" should be right after includes from "catalog/" to follow the alphabetic order (unless there's a reason why that does not work).
Fixed.
- I rather dislike the "dim" parameter name, because I immediately think "dimension" which makes little sense. I suggest renaming to "nitems" or "nelements" or something like that.
Agreed, using "nelements" as a better style matchup.
Please, find attached a version addressing the above mentioned.
