pasa / asdl-rs

Apache License 2.0
10 stars 1 forks source link

Replace borrowing with owned Strings #3

Closed matklad closed 5 years ago

matklad commented 5 years ago

Currently, AST is parametrized over 'a and borrows from the original string. Given that ASDL files are small, and should be used only during the build phase, I think we should optimize for convenience and not performance in this case.

Let's make AST owned (ie, store String instead of 'a str).

pasa commented 5 years ago

Currently, AST is parametrized over 'a and borrows from the original string. Given that ASDL files are small, and should be used only during the build phase, I think we should optimize for convenience and not performance in this case.

Let's make AST owned (ie, store String instead of 'a str).

I certainly can do it. But AST structures are private right now. I figured out that raw ASDL AST is too difficult to process in template engines. I've implemented an ASDL model to fix this. This model has flat structure instead of tree. Generates missed filed names (asdl allows fileds without names). It has owned Strings to make it simple.

Perhaps, I miss something and I need to make AST public? In this case your concern about borrowed AST is really true.

matklad commented 5 years ago

Ah, indeed, AST is pub-crate, I've missed this fact.

Yeah, the model looks like a better API here, but that makes me think whether we need a separate ast at all here... Can't we just parse directly into the model?

pasa commented 5 years ago

We certainly can. There are some reasons for current design I had in mind. First of all I wanted to keep AST types like a raw asdl syntax representation. So model can be changed in future or maybe it will be second one, but AST keep unchanged. Second this is a kind of example of usage. AST types are generated by this crate facilities from asdl file. So to make separate AST types less memory influencing I chose zero copy &str borrowing implementation.

I'm quote a newbie i rust. May be the are some reasons not to do like this?

matklad commented 5 years ago

Yeah, these are all valid reasons to keep model & ast separate, closing then!