ibireme / yyjson

The fastest JSON library in C
https://ibireme.github.io/yyjson/doc/doxygen/html/
MIT License
2.98k stars 262 forks source link

Condition for calling read_number_raw #170

Closed danielaparker closed 2 weeks ago

danielaparker commented 3 weeks ago

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 commented 3 weeks ago

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.

danielaparker commented 3 weeks ago

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 commented 2 weeks ago

Fixed