Removed a static variable used to contain the current recursion depth in json_reader.cpp #556
Conversation
Removed a static variable used to contain the current recursion depth of Reader::readValue(). The number of elements in an internal container Reader::nodes_ is used instead. It is correct because any recursive call of Reader::readValue() is executed with adjacent nodes_.push() and nodes_.pop() calls. Added the option to change the allowed recursion depth at compile time by defining a macro JSONCPP_STACK_LIMIT as the required integer value.
| if (stackDepth_ >= features_.stackLimit_) throwRuntimeError("Exceeded stackLimit in readValue()."); | ||
| ++stackDepth_; | ||
| // To preserve the old behaviour we cast size_t to int. | ||
| if (static_cast<int>(nodes_.size()) > features_.stackLimit_) throwRuntimeError("Exceeded stackLimit in readValue()."); |
There was a problem hiding this comment.
I don't understand what was wrong with OurReader. Could you explain?
There was a problem hiding this comment.
stack<>.size() complexity is constant: stack is deque (http://www.cplusplus.com/reference/stack/stack/) and deque<>.size complexity - http://www.cplusplus.com/reference/deque/deque/size/
About OurReader. I have change it because of the following comment to the class:
// exact copy of Reader, renamed to OurReader
and
// complete copy of Read impl, for OurReader
If you allow some small differences I would rollback my changes to OurReader.
There was a problem hiding this comment.
Yes, it is ok for OurReader to differ.
There was a problem hiding this comment.
@cdunn2001 there's no sense to keep stackDepth_ in OurReader. New solution uses less additional memory (it's just a single member but I believe it's important for embedded devices) and still has the same computational complexity. And the tests are OK.
There was a problem hiding this comment.
I see your point. Thanks.
| #else | ||
| static int stackDepth_g = 0; // see readValue() | ||
| // Define JSONCPP_STACK_LIMIT as an appropriate integer at compile time to change the stack limit | ||
| #if !defined(JSONCPP_STACK_LIMIT) |
There was a problem hiding this comment.
I think this should be JSONCPP_DEPRECATED_STACK_LIMIT, since this should apply only to the old deprecated Reader, which nobody should be using anyway.
There was a problem hiding this comment.
About JSONCPP_DEPRECATED_STACK_LIMIT: Ok. I will make the reqiured changes.
Actually there are some product, which used jsoncpp in the old manner. Replacing a dll is relatively simple, changing all modules may be considerable harder.
Renamed JSONCPP_STACK_LIMIT to JSONCPP_DEPRECATED_STACK_LIMIT to stress that usage of this macros assumes old interface.
Removed a static variable used to contain the current recursion depth of Reader::readValue(). The number of elements in an internal container Reader::nodes_ is used instead. It is correct because any recursive call of Reader::readValue() is executed with adjacent nodes_.push() and nodes_.pop() calls.
Added the option to change the allowed recursion depth at compile time by defining a macro JSONCPP_DEPRECATED_STACK_LIMIT as the required integer value.