AST source locations #16
Conversation
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct Loc<X>(pub X, pub Source); |
There was a problem hiding this comment.
Does it help in any way to absorb the Box<_> around the X into this type definition?
That way, we avoid having to think about the Box and the Loc separately, and they combine in the AST structure.
There was a problem hiding this comment.
I originally included Box<_> in the definition, but I ran into edge cases where I just wanted the source information without storing everything on the heap (specifically for the lexer). This could also give us room for optimizations down the line in the interpreter.
src/lib/ast.rs
Outdated
|
|
||
| pub type Prog = Decs; | ||
|
|
||
| pub type Dec_ = Loc<Box<Dec>>; |
There was a problem hiding this comment.
I wonder if reading Loc<Dec> is simpler, where it would imply the box?
See my earlier comment about this question, near the def for Loc.
There was a problem hiding this comment.
Makes sense for the AST but adds a bunch of extra overhead to the lexer. If this is enough of a QoL issue, we could maybe add a standard type alias called LB<_> or similar.
We could also add some specialized sugar functions by adding and impl<X> for Loc<Box<X>>, which makes it seem okay to have the optional heap allocation from an abstraction standpoint.
src/lib/parser_utils.rs
Outdated
| vec: vec![d], | ||
| has_trailing: false, | ||
| pub fn dec_into_exp(d: Dec_) -> Exp_ { | ||
| Loc( |
There was a problem hiding this comment.
Same question, and more evidence for combining I think.
There was a problem hiding this comment.
Here's a counterexample where it makes sense to use Loc<_> without Box<_>. We could also eventually use this to optimize the VM, since boxing would add a bit of extra overhead which could make an impact on the interpreter's performance. Seems worth having a "separation of concerns" similar to Rust's Arc<Mutex<_>> pattern for reusability in different situations. I'll definitely add a shorthand for Loc<Box<_>> though, since it will probably continue to be used in most cases.
|
This is looking really good, and very tedious in terms of the changes required. Thank you for undertaking it! |
|
Replaced all usages of |
|
Accidentally converted from "draft" to "ready to review", so just reverted that. |
Resolves #7.
Progress:
Exp_,Type_, etc. to include source locations