search

parsingdata / metal

A Java library for parsing binary data formats, using declarative descriptions.
Apache License 2.0
18 stars 9 forks source link

#399 Introducing ParseValueCache. #401

Closed mvanaken closed 1 year ago

mvanaken commented 1 year ago

A cache is added to the parse state as a lookup map for values by name. The cache is used where possible. If it is unable to use the cache, the normal lookup through the parse graph is used, which is the original behaviour.

This only creates a lookup map by name, not by token. @ reviewers, do we want to include that?

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (a9a9fb2) 100.00% compared to head (de0c782) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #401 +/- ## =========================================== Coverage 100.00% 100.00% - Complexity 1127 1151 +24 =========================================== Files 97 98 +1 Lines 1490 1529 +39 Branches 154 158 +4 =========================================== + Hits 1490 1529 +39 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rdvdijk commented 1 year ago

I haven't reviewed this yet (I will, Soon™), but I have a question: Does the ParseValueCache work nicely with Scope?

Last week we (successfully) used Scope, and I immediately thought of this new cache.

(You might already have added a test for this, please ignore me if you have.)

mvanaken commented 1 year ago

I haven't reviewed this yet (I will, Soon™), but I have a question: Does the ParseValueCache work nicely with Scope?

Last week we (successfully) used Scope, and I immediately thought of this new cache.

(You might already have added a test for this, please ignore me if you have.)

Yes and no. :blush: Yes, because everything still works as expected when using scope. And no, because the cache is turned off when evaluating scoped value expressions, see also my comment here.

I have added a test, see my other comment here, to test when the cache is used and when it is not. For all scoped variants it is turned off.