Conversation
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
LGTM with a nit and a question.
| // Keep a record of call depth, to guard against excessive recursion. | ||
| size_t depth = 0; | ||
| Index maxDepth; | ||
|
|
There was a problem hiding this comment.
nit: this whitespace looks unnecessary.
There was a problem hiding this comment.
It's sort of separating the input values from the internal state values. Is there a clearer way to do that? I could use another protected: block lower down I guess.
There was a problem hiding this comment.
Nope, this newline seems fine if it's there intentionally 👍
| // that we can do anything useful to precompute a hugely nested expression | ||
| // (we should succed at smaller parts of it first). Second, a low limit is | ||
| // helpful to avoid platform differences in native stack sizes. | ||
| enum { MAX_DEPTH = 50 }; |
There was a problem hiding this comment.
It could be a static const or a #define I guess. I don't have a preference, what do you think?
There was a problem hiding this comment.
I would default to using a static const. That way the underlying type is clear and there are no extra type shenanigans happening.
2 participants
Keep limiting in precompute as before: that is useful since that pass is run as part of normal compilation, and we want to avoid native stack limits on all platforms. Also that pass is not likely to find any pattern of size 50 of higher that it can't precompute as a sum of smaller pieces.
Restore the 250 limit from before for interpreting entire modules, as without that the fuzzer will sometimes hit the limit and cause a false positive.