Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Condition for calling read_number_raw #170

Closed
danielaparker opened this issue Jun 27, 2024 · 3 comments
Closed

Condition for calling read_number_raw #170

danielaparker opened this issue Jun 27, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@danielaparker
Copy link
Contributor

There must have been a reason for using

    /* read number as raw string if has `YYJSON_READ_NUMBER_AS_RAW` flag */
    if (unlikely(pre && !has_read_flag(BIGNUM_AS_RAW))) {
        return read_number_raw(ptr, pre, flg, val, msg);
    }

rather than simply

    if (unlikely(has_read_flag(READ_NUMBER_AS_RAW))) {
        return read_number_raw(ptr, pre, flg, val, msg);
    }

but I don't see it.

One, it doesn't seem to be in the spirit of yyjson to do two checks where one will do.

Two, the documentation for YYJSON_READ_BIGNUM_AS_RAW states "The flag will be overridden by YYJSON_READ_NUMBER_AS_RAW flag." But with this code, it's the other way around, if YYJSON_READ_BIGNUM_AS_RAW | YYJSON_READ_NUMBER_AS_RAW is passed as flags, it's YYJSON_READ_BIGNUM_AS_RAW that overrides YYJSON_READ_NUMBER_AS_RAW.

It's a precondition that pre is not null before the call to read_number_raw, but that's already guaranteed if either of the flags are set.

All tests still pass with the alternative check.

@ibireme
Copy link
Owner

ibireme commented Jun 28, 2024

Thanks for your insight!
This condition was added along with YYJSON_READ_BIGNUM_AS_RAW, but it doesn't match the documentation.
This should be considered a bug and not covered by the tests.

@ibireme ibireme added the bug Something isn't working label Jun 28, 2024
@danielaparker
Copy link
Contributor Author

danielaparker commented Jun 28, 2024

My own view is that the documentation accords with intuition - that YYJSON_READ_NUMBER_AS_RAW | YYJSON_READ_BIGNUM_AS_RAW should return all numbers as raw - and that it's the condition that should be changed.

ibireme added a commit that referenced this issue Jun 30, 2024
@ibireme
Copy link
Owner

ibireme commented Jun 30, 2024

Fixed

@ibireme ibireme closed this as completed Jun 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants