search

dgkf / R

An experimental reimagining of R
https://dgkf.github.io/R
GNU General Public License v3.0
136 stars 5 forks source link

Replace `Expr::ExprList` with `Obj::List` #151

Open sebffischer opened 3 months ago

sebffischer commented 3 months ago

As @dgkf suggested in the discussion here: https://github.com/dgkf/R/pull/146, we can replace ExprList by just using an Obj::List with expressions.

sebffischer commented 3 months ago

This would mean, however, that there is an additional indirection because currently the list directly contains Expr and would then contain Obj::Expr. we might want to make List generic.

dgkf commented 3 months ago

Yeah - that's certainly one concern I have. Representing ExprList as List<Obj> means that we need to do some dispatch on the Obj enum (or maybe in the future, trait) even though we know that every element is a Expr. For something as central as the language AST, this would definitely make it annoying to program around and perhaps come with a small performance hit.

We might be going off on an abstraction detour, but we could also handle Lists as heterogenous Vectors and ExprList as another homogeneous Vector<Expr> to avoid the dispatch and clean up the internal api for working with the AST.

sebffischer commented 3 months ago

We might be going off on an abstraction detour, but we could also handle Lists as heterogenous Vectors and ExprList as another homogeneous Vector<Expr> to avoid the dispatch and clean up the internal api for working with the AST.

Yeah, I was already wondering this for the List, let's see how easy that refactor is.

sebffischer commented 3 months ago

If we represent List as Vector<Obj>, we would thereby also solve https://github.com/dgkf/R/issues/105, so maybe this is the right call.

sebffischer commented 3 months ago

We might be going off on an abstraction detour, but we could also handle Lists as heterogenous Vectors and ExprList as another homogeneous Vector<Expr> to avoid the dispatch and clean up the internal api for working with the AST.

Maybe this then also means that we have to refactor Vector from an enum to a Vector<T> generic. Thereby we could then conveniently use Vector<Expr> for ExprList and Vector<Obj> for List, while still not mixing ExprList, List and the other atomic vectors too much in the code base.

If you think this is a good idea, I can give this a try.

sebffischer commented 3 months ago

@dgkf Before I invest time in this it would be great to get your feedback on whether you would be okay with refactoring the Vector enum into a generic Vector<T>. This would mean that the Obj::Vector variant would become Obj::Character, ..., Obj::Double. I think it would allow us to solve https://github.com/dgkf/R/issues/105#issuecomment-2261845031, which is on the v0.4 milestone

dgkf commented 3 months ago

Fundamentally I have no issue with testing it out. I think we should be able to go this route even if we kept the current Obj::Vector(Vector::{Double,Character,..}) as well as Obj::List(List) where List is just a type alias for Vector<Obj>.

I lean toward this style because it will make it easier to differentiate Scalar and Array types when we get to those. If there are other benefits, then just be mindful to consider how data with different dimensions should be represented in the Obj enum.

And just regarding the milestone - I tried to tag things that looked realistic, but regardless of what makes the cut I plan to do the release in the next days. Since I don't think this is a realistic timeframe for this feature, I'm going to bump this one to 0.4.1