We discussed this a bit, focusing on three points:
- Avoid shared_ptr to make it possible for entries to be empty; use optional instead.
- If the caching functionality is added by the base class, its implementation should also live in the base class.
- The change makes the previously logically immutable scoring function classes mutable, which will lead to complications once we move to builders.
I'm OK to merge this, but these points should be addressed in followup issues. In particular, it looks like we add an option for caching to all scoring functions, but if you use it for anything other than MIASM, it leads to an error. If the option must not be used, this perhaps should be mentioned in the documentation as well (and then it's perhaps easier to either add it just to MIASM or support it for all from the start).
The new change also imposes new requirements on scoring functions, i.e., that they don't change in a way that would invalidate the caching. I think it would be good for this to be clearly documented in our code, i.e., what is the contract for scoring functions.
|