Hi, Thanks for reviewing this patch.
On Mon, Dec 9, 2024 at 3:07 AM Tomas Vondra <to...@vondra.me> wrote: > Thanks for a nice patch. I did a quick review today, seems almost there, > I only have a couple minor comments: > > 1) Template Patterns for Numeric Formatting > > Why the wording change? "input between 1 and 3999" sounds fine to me. > input... seemed to refer to numeric input for to_char roman format. But, after roman support in to_number function shouldn't we modify it? It is the reason I changed it to "valid for numbers 1 to 3999". 2) The docs say "standard numbers" but I'm not sure what that is? We > don't use that term anywhere else, I think. Same for "standard roman > numeral". > I will change "standard numbers" to "numbers". Note that we are following the standard form. There are other forms too. For example, 4 can be represented as IV (standard) or IIII (non standard). https://en.wikipedia.org/wiki/Roman_numerals#Standard_form > 3) A couple comments need a bit of formatting. Multi-line comments > should start with an empty line, some lines are a bit too long. > I will fix the multi-line formatting. > 4) I was wondering if the regression tests check all interesting cases, > and it seems none of the tests hit these two cases: > > if (vCount > 1 || lCount > 1 || dCount > 1) > return -1; > SELECT to_number('viv', 'RN') test covers this if statement for the subtraction part. But, I noticed that no test covers simple counts of V, L, D. I will add SELECT to_number('VV', 'RN') for that. > and > > if (!IS_VALID_SUB_COMB(currChar, nextChar)) > return -1; > Noted. SELECT to_number('IL', 'RN') test can be added. Regards, Hunaid Sohail
