Accept large unsigned integers - #295
Conversation
|
@saka1 Can you have a look at the failing tests? |
|
Oh... I'll try to fix 😱 |
|
Looks like we are passing the tests. That's great. Now we just need independent review (performance, correctness). If anyone wants to jump in, please do. Otherwise, this is on my todo. |
|
|
||
| inline bool is_integer() const { return get_type() == 'l'; } | ||
|
|
||
| inline bool is_unsigned_integer() const { return get_type() == 'u'; } |
There was a problem hiding this comment.
should it be any positive integer?
In that case, I think I could easily add it to the fuzzer.
There was a problem hiding this comment.
Hmm, your suggestion is like below?
inline bool is_unsigned_integer() const {
return get_type() == 'u' || (is_integer() && get_integer() >= 0);
}This behavior seems consistent. But there is a little performance tradeoff.
Even if this is_unsigned_integer() returns true, only safe way to read the value is get_unsigned_integer() because get_integer() happens overflow for a large unsigned integer.
In such a situation, I think it's enough to check if the element type is 'u', so (is_integer() && get_integer() >= 0) is redundant.
Even so, should I do it?
There was a problem hiding this comment.
There was a problem hiding this comment.
well, in fact I was asking if we wanted a behaviour similar to rapidjson's. Yes, something like what you described. Seems like it does not matter. In any case, Daniel did point out a real issue. The correction looks good to me.
I also thought that a behaviour like rapidjson's would be easier to test with the fuzzer. On a second thought, it should not be a lot more complicated.
|
Thanks @ioioioio Help appreciated and further help invited! |
|
Still on my todo. |
* Add integer_tests * Add get_unsigned_integer() on ParsedJson::BasicIterator
|
@lemire |
|
@saka1 Don't worry, we won't block PR based on failed perf tests. We get many false positives. Sadly. |
|
(We do assess performance, but right now it must be done manually... the CI is just interpreted as a warning.) |
|
Still on my todo to review this. |
|
I have added unsigned integers to the fuzzer: https://github.com/ioioioio/fuzzyjson/tree/add_uint Ran the thing many times. Saw many big uint64. Everything went fine. |
|
@ioioioio Thank you. So we are going to assume that it is correct. I want to do a manual review before merging. |
|
@saka1 I am about to merge a slightly modified version of this PR. I would credit your name in the CONTRIBUTORS file, but I would need your actual name. You can issue a PR directly if you'd like. |
* handle uint64 value in JSON * Add integer_tests * Add get_unsigned_integer() on ParsedJson::BasicIterator * Write 'u' to tape when the value seems unsigned * Add to handle 'u' element * Brush up integer_tests.cpp * Append tests/integer_tests in .gitignore * Add comments to is_integer and is_unsigned_integer
This PR solves #68.
As far as I see, a new element type of the tape is necessary to handle unsigned large (> 2^63) value.
So I add the 'u' element for such values.