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