DynMap #2484
DynMap #2484
Conversation
|
The main idea here is that
|
| @@ -64,6 +68,12 @@ impl<N: AstNode> PartialEq for AstPtr<N> { | |||
| } | |||
| } | |||
|
|
|||
| impl<N: AstNode> Hash for AstPtr<N> { | |||
| fn hash<H: Hasher>(&self, state: &mut H) { | |||
| self.raw.hash(state) | |||
There was a problem hiding this comment.
what's the difference to the derived implementation here? 🤔
There was a problem hiding this comment.
Derive would be impl<N: AstNode + Hash>
| @@ -33,7 +33,9 @@ fn type_at_pos(db: &TestDB, pos: FilePosition) -> String { | |||
| let expr = algo::find_node_at_offset::<ast::Expr>(file.syntax(), pos.offset).unwrap(); | |||
| let fn_def = expr.syntax().ancestors().find_map(ast::FnDef::cast).unwrap(); | |||
| let module = db.module_for_file(pos.file_id); | |||
| let func = module.child_from_source(db, InFile::new(pos.file_id.into(), fn_def)).unwrap(); | |||
| let func = *module.child_by_source(db)[keys::FUNCTION] | |||
| .get(&InFile::new(pos.file_id.into(), fn_def)) | |||
There was a problem hiding this comment.
I don't quite understand why the API is always building a whole map when it seems like all uses just immediately do get on it (maybe I missed something)
There was a problem hiding this comment.
Building the whole map is easier: you just traverse everything and put it into the map. If you were directly looking for a specific Item, you would have first filter by item type and, in general, provide separate code for each type of looked for item. See the deleted child_from_source module, it pursued that approach.
The map should also be cache able in theory, so that repeated lookups for neighboring nodes will be fast.
There was a problem hiding this comment.
Maybe it makes sense to change () -> DynMap to (&mut DynMap) though: that way, we can change the API in the future to use a DynMap which drops unrelated keys on the floor.
crates/ra_hir_def/src/dyn_map.rs
Outdated
| } | ||
|
|
||
| impl<K, V, P> Key<K, V, P> { | ||
| #[allow(unused)] |
There was a problem hiding this comment.
this is used below, so is the allow necessary?
| fn clone(&self) -> Key<K, V, P> { | ||
| *self | ||
| } | ||
| } |
There was a problem hiding this comment.
does derive not work for these impls?
There was a problem hiding this comment.
It won't for Copy, because K and V are generally not Copy
|
So, @flodiebold what's your feeling here? Is this in general a worthwhile thing to try? |
|
Sure, I think it's worth a try! |
|
bors r+ |
2484: WIP: DynMap r=matklad a=matklad Implement a `DynMap` a semi-dynamic, semi-static map, which helps to thread heterogeneously typed info in a uniform way. Totally inspired by https://github.com/JetBrains/kotlin/blob/df3bee30384787d8951ea548a4257c2cb52a16a3/compiler/frontend/src/org/jetbrains/kotlin/resolve/BindingContext.java. @flodiebold wdyt? Seems like a potentially useful pattern for various source-map-like things. Co-authored-by: Aleksey Kladov <[email protected]>
Canceled |
|
bors r+ |
2484: DynMap r=matklad a=matklad Implement a `DynMap` a semi-dynamic, semi-static map, which helps to thread heterogeneously typed info in a uniform way. Totally inspired by https://github.com/JetBrains/kotlin/blob/df3bee30384787d8951ea548a4257c2cb52a16a3/compiler/frontend/src/org/jetbrains/kotlin/resolve/BindingContext.java. @flodiebold wdyt? Seems like a potentially useful pattern for various source-map-like things. Co-authored-by: Aleksey Kladov <[email protected]>
Build succeeded
|
Implement a
DynMapa semi-dynamic, semi-static map, which helps to thread heterogeneously typed info in a uniform way. Totally inspired by https://github.com/JetBrains/kotlin/blob/df3bee30384787d8951ea548a4257c2cb52a16a3/compiler/frontend/src/org/jetbrains/kotlin/resolve/BindingContext.java.@flodiebold wdyt? Seems like a potentially useful pattern for various source-map-like things.