Conversation
* Make the code more thread safe * Do a bounds check in the GC closure Not sure if the 2nd is needed, or if we have an assumption that `lookupSeqMap` will only be called with in bounds values---this is something we should investigate and document.
| -- this code should never run (unless we messed up something). | ||
| evalPanic i = case sz of | ||
| Nat sz' | i < 0 || i >= sz' -> invalidIndex sym i | ||
| _ -> panic "lookupSeqMap" |
There was a problem hiding this comment.
Nit pick: this function is no longer located in lookupSeqMap, so it would be worth changing the string used in the call to panic here.
| !(Integer -> SEval sym a) | ||
| -- Use this to overwrite the evaluation function when the cache is full |
There was a problem hiding this comment.
I'm not entirely sure why we need to add this as an additional field to MemoSeqMap. Unless I'm missing something, the only place where we ever construct MemoSeqMap values is in the memoMap function, and memoMap only ever instantiates this field to evalPanic. Perhaps we should just turn evalPanic into a top-level function and refer to that instead?
There was a problem hiding this comment.
I had to add it there, because evalPanic (which perhaps should be renamed) throws an exception if the index is out of bounds, and that needs access to the backend sym. Unfortunately, sym is not passed to lookupSeqMap, so instead of changing the API I just added the function to use when the cache is complete.
I imagine passing in the sym would not be a huge change though, so perhaps we should do that instead.
One other thing I am not sure, and we really should document, is what are the invariants on lookupSeqMap. In particular, I am not sure if it might be assuming that someone has already checked that the index is in bounds, so maybe we don't even need that exception... I added it because in the original ticket it looked like that map size is 2, and the index is 2, so that would make it out of bounds, which would suggest that we don't have such an invariant.
3 participants
Not sure if the 2nd is needed, or if we have an assumption that
lookupSeqMapwill only be called with in bounds values---this is something we should investigate and document.