Re: [PATCH] Add roman support for to_number function

Hi,

On 10/30/24 08:07, Hunaid Sohail wrote:
> Hi,
>
> I have attached a rebased patch if someone wants to review in the next
> CF. No changes as compared to v4.
>
> Regards,
> Hunaid Sohail
>

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.

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".

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.

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;

and

if (!IS_VALID_SUB_COMB(currChar, nextChar))
return -1;

I haven't tried constructing tests to hit those cases, though.

Seems ready to go otherwise.

regards

--
Tomas Vondra