Remove the trait dispatch from memoized.
#333
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was inherited from Salsa but had no real use, and we've moved a bit away from Salsa now. In particular, there's various comments to the effect of "maybe having a trait will help us with separate compilation someday?" -- but we ended up doing separate compilation in a much more fine-grained way, with one function pointer per method. So the trait _still_ doesn't help with that. This does make constructing a totally fake instance in rs_snippet much more annoying. What it used to do was "crash if you use the database". It still does that, but via a local test injectiony dependency-injectiony thing, not by using traits. Overall, it's about the same, but maybe more visibly awkward. We should probably move away from this pattern, and I think I can/will for the display part at least, in a followup. Benefits of getting rid of the trait objects: 1. No more `dyn`! Yes! Hooray! This both gives us flexibility improvements (can deal in concrete types, Sized, etc), and presumably marginally improves performance or something. 2. Overall simpler -- just one type, `BindingsGenerator`, instead of two types. We in the past had problems where one function accepted BindingsGenerator and the other accepted Database and then they could only talk in one direction. 3. Now that we're committing to dealing in concrete types, we have room to e.g. produce new BindingsGenerator objects by value. We could do that from the trait, too, but only if we were committing to the trait being pointless. This, in turn, means we can get rid of global state, and instead reuse BindingsGenerator as you'd expect: make a new BindingsGenerator object, with new state. PiperOrigin-RevId: 860318906
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove the trait dispatch from
memoized.This was inherited from Salsa but had no real use, and we've moved a bit away from Salsa now. In particular, there's various comments to the effect of "maybe having a trait will help us with separate compilation someday?" -- but we ended up doing separate compilation in a much more fine-grained way, with one function pointer per method. So the trait still doesn't help with that.
This does make constructing a totally fake instance in rs_snippet much more annoying. What it used to do was "crash if you use the database". It still does that, but via a local test injectiony dependency-injectiony thing, not by using traits. Overall, it's about the same, but maybe more visibly awkward. We should probably move away from this pattern, and I think I can/will for the display part at least, in a followup.
Benefits of getting rid of the trait objects:
No more
dyn! Yes! Hooray! This both gives us flexibility improvements (can deal in concrete types, Sized, etc), and presumably marginally improves performance or something.Overall simpler -- just one type,
BindingsGenerator, instead of two types. We in the past had problems where one function accepted BindingsGenerator and the other accepted Database and then they could only talk in one direction.Now that we're committing to dealing in concrete types, we have room to e.g. produce new BindingsGenerator objects by value. We could do that from the trait, too, but only if we were committing to the trait being pointless.
This, in turn, means we can get rid of global state, and instead reuse BindingsGenerator as you'd expect: make a new BindingsGenerator object, with new state.