Skip to content
This repository was archived by the owner on Nov 22, 2018. It is now read-only.

Emit Go AST instead of plain text #13

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

dennwc
Copy link

@dennwc dennwc commented May 28, 2018

Refactor the code to build Go AST instead of printing raw text. This will allow to build all optimizations directly into AST helpers and will allow to simplify the code further.

All tests pass, the only difference with original generated Go code - it's missing comments for arguments (see golang/go#20744).

Based on #12 branch.

@cznic
Copy link
Owner

cznic commented May 28, 2018

@dennwc

I'm sorry I haven't yet found time to respond to the previsous PRs, (thanks for them!), so this is my fault. I mean, we need to discuss major/bigger changes before implementing them. Bug fixes are a different thing of course.

I'll try to catch-up with #12, hopefully today or tomorrow, but for this issue, I'm frankly not convinced that generating Go AST instead of generating text is a good idea. Please summarize what benefit it may have, because I'm not aware of any. OTOH, getting the AST from text is easy whenever/if needed - see for example opt.go.

Once again my apologies for late replying. It may have avoided possibly wasting your effort. I'd be terribly sorry for that to happen.

PS: I've now skimmed the CL, it's huge. This will take a lot of time for me to dig into in any case.

@dennwc
Copy link
Author

dennwc commented Jun 2, 2018

@cznic Yes, this CL looks huge, but if you take a closer look at the actual changes, it literally replaces text writes with appends to either []ast.Decl or []ast.Stmt.

I want to first explain why I did it. I examined the code and found many repetitive text patterns like unsafe pointer access. I made few helpers for these code samples so they are generated in a single function. After doing it for every pattern I found I realized that they exactly match AST node bounds, and all this helper code does is replicating go/printer which seems redundant. This why I changed it in the first place. But there is a strategic reason as well.

Regarding actual benefits, it will allow moving all code in opt.go into these AST helpers, so they will build the optimal code on the first try, without the need to parse the file again. I know that the project is in alpha stage and this kind of optimization seems premature. But it might actually simplify the code in other code paths - it will be able to examine current Go AST and generate its part accordingly. For example, code generator might be able to examine current scope of the code block (that might be different from one presented in C code) to make a decision about the code that needs to be generated. Another example - it's possible to add declarations or initialization code in the process of emitting AST, and not as a preprocess step that leaves _ = a kind of statements. It makes the generated code much cleaner, and allow to generate it in a single pass.

Also, from my experience, I can compare the maintainability of two protobuf code generators: gogo and proteus. I made a few medium-size PRs to both and can state that adding modules to projects that work on AST level is much easier. Keeping in mind that a number of tweaks will only grow, I think it makes sense to switch to a model that will allow to maintain them more easily. In general, text code generators tend to bake in all variable names into a single code string making it harder for modules to alter the resulting code. On the other hand, AST generators can pass pointers to nodes, and allow to examine the scope of these nodes while generating the code.

I hope it explains the approach I took and justifies an effort.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants