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
