Skip to content

Parsing incorrect if number text has thousands separator(s)#110

Closed
nietras wants to merge 1 commit intoCarlVerret:masterfrom
nietras:thousand-separator-fails
Closed

Parsing incorrect if number text has thousands separator(s)#110
nietras wants to merge 1 commit intoCarlVerret:masterfrom
nietras:thousand-separator-fails

Conversation

@nietras
Copy link
Copy Markdown
Contributor

@nietras nietras commented Apr 25, 2026

I was not aware of this, but it appears csFastFloat does not parse numbers correctly if text has thousands separators? Is that not handled in fast_float? The test fails for "1,234,567.89" with:

Assert.Equal() Failure
Expected: 1234567.9
Actual:   1

@nietras
Copy link
Copy Markdown
Contributor Author

nietras commented Apr 25, 2026

another issue is that whitespace characters are not counted towards characters consumed, not sure if reason for not doing this?

    internal unsafe static bool TryParseNumber(char* first, char* last, out int characters_consumed, out float result, NumberStyles styles = NumberStyles.Float, char decimal_separator = '.')
    {
        characters_consumed = 0;
        result = 0f;
        while (first != last && Utils.is_ascii_space(*first))
        {
            first++;
        }

        if (first == last)
        {
            result = 0f;
            characters_consumed = 0;
            return false;
        }

first moved is later not added to characters consumed

@lemire
Copy link
Copy Markdown
Collaborator

lemire commented Apr 25, 2026

Thanks. I’ll have a look.

I do not expect thousand separators to work: I don’t think that it was ever was advertised.

@nietras
Copy link
Copy Markdown
Contributor Author

nietras commented Apr 26, 2026

I do not expect thousand separators to work: I don’t think that it was ever was advertised.

No it isn't, however I made the mistake of thinking this meant parsing would fail/return false for case with such characters in text, I have changed to check consumed chars and if text contains thousand separators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants