-
Notifications
You must be signed in to change notification settings - Fork 56
Open
Description
There is a behaviour that is unconsistent with the comment here:
jmespath.php/src/AstRuntime.php
Lines 37 to 42 in a2a865e
| // Clear the AST cache when it hits 1024 entries | |
| if (++$this->cachedCount > 1024) { | |
| $this->cache = []; | |
| $this->cachedCount = 0; | |
| } | |
| $this->cache[$expression] = $this->parser->parse($expression); |
- After the first call
cachedCountwill be 1 and the cache will have 1 entry. - After the 1024th call
cachedCountwill be 1024 and the cache will have 1024 entries.
👉Note that while the cache has hit 1024 entries it is not cleared yet, so to me the comment is already a little misleading. - After the 1025th call
cachedCountwill be 0 and the cache will have 1 entry.
👉One could say that the state of the object is now invalid ? - After the 2048th call
cachedCountwill be 1023 and the cache will have 1024 entry.
👉Note that while the cache has hit 1024 entries it is not cleared yet, once again. - After the 2049th call
cachedCountwill be 1024 and the cache will have 1025 entry.
❌This is more than 1024 and to me is not what was expected by the developper who wrote the comment. - After the 2050th call
cachedCountwill be 0 and the cache will have 1 entry.
👉Again, one could say that the state of the object is now invalid - (Then it will repeat)
Please have a look to the code illustrating the current behaviour on 3v4l:
https://3v4l.org/MsW7e
Side proposition : rewrite the comment
- // Clear the AST cache when it hits 1024 entries
+ // Clear the AST cache if it already holds 1024 entries Proposition 1 : fastest fix
- $this->cachedCount = 0;
+ $this->cachedCount = 1;Proposition 2 : cleaner code (as in "easier to review")
- if (++$this->cachedCount > 1024) {
+ if ($this->cachedCount > 1024) {
$this->cache = [];
$this->cachedCount = 0;
}
$this->cache[$expression] = $this->parser->parse($expression);
+ $this->cachedCount++;Metadata
Metadata
Assignees
Labels
No labels