Provide provenance info for llvm::Uses
#246
Conversation
|
Needs #247 to pass AnghaBench |
* Add regression test * Fix overzealous propagation
There was a problem hiding this comment.
Generally looks okay and I get what you're trying to do with the "SSAification" of the AST.
What I have qualms about is the mingling of "IR-to-C translation code" and "structurization code". IRToASTVisitor.(h|cpp) was meant to be the place where the IR translation code lived. GenerateAST.(h|cpp) was for structurization. The BlockVisitor class I think breaks this distinction. Personally, I would like for this kind of distinction to exist. One reason, because it makes it easy to reference the No More Gotos paper back and forth. Another reason is purely for conceptual partitioning of the code.
The distinctions don't have to be "translation" and "structurization". If you can come up and describe (optionally document) the conceptual distinctions and they make sense, anything is fine by me.
|
I've put |
There was a problem hiding this comment.
As much as I don't like to bloat up IRToASTVisitor.(h|cpp) I think it makes most sense to put BlockVisitor there. Maybe even move clang::Expr *visitXYZ methods into a separate class, named something like ExprGen. Move clang::Stmt *visitXYZ methods from BlockVisitor into StmtGen. And finally make the IRToASTVisitor class use both. The logic from BlockVisitor::visitBasicBlock could be put into a StmtVec IRToASTVisitor::CreateBasicBlockStmts(llvm::BasicBlock&) or similar. What do you think?
lib/AST/IRToASTVisitor.cpp
Outdated
| ExprGen(clang::ASTUnit &unit, Provenance &provenance) | ||
| : ast_ctx(unit.getASTContext()), ast(unit), provenance(provenance) {} | ||
|
|
||
| clang::QualType GetQualType(llvm::Type *type) { |
There was a problem hiding this comment.
Can we factor out method bodies like GetQualType out of the class declaration? Again, no other but for consistency and/or neatness's sake. This concerns both ExprGen and StmtGen. I promise this is the last piece of annoyance 👀
There was a problem hiding this comment.
Do you mean making them static or separating all the declarations of ExprGen and StmtGen from their definitions?
There was a problem hiding this comment.
Just separating. The class declarations would only have method declarations and outside of the class declaration, you would have the definitions, i.e. clang::Expr *ExprGen::visitBinaryOperator(llvm::Instruction &inst) {...}.
|
Had to put |
Neat! Think we can merge? |
|
Yep, tests are passing! |
Also fixes #194 and #205