v.g.vassilev added subscribers: sunho, sgraenitz, lhames.
v.g.vassilev added a comment.
I believe this is heading in a good direction. Here are some comments from a
partial review. Also perhaps we should extend the patch description (future
commit message) to capture the idea of the pretty-printing including the
dispatch, the approach how we build the AST and how to write a custom printer.
================
Comment at: clang/lib/Interpreter/Value.cpp:272
+
void Value::print(llvm::raw_ostream &Out) const {
assert(OpaqueType != nullptr && "Can't print default Value");
----------------
We should add some documentation for users how to write a pretty-printer for a
custom class.
We do not seem to support the multiple inheritance case where one of the base
classes has a pretty-printer and the other does not. See the explanation in
Cling. It is okay to not have it at the moment but we should include a FIXME
here saying that we do not yet support it.
================
Comment at: clang/lib/Interpreter/ValuePrinter.cpp:201
+ clang::Parser::ParseScope PS(
+ &Interp.getParser(), clang::Scope::FnScope | clang::Scope::BlockScope);
+
----------------
Similarly to the way we rebuild the AST during template instantiation, when we
are synthesizing AST we do not need the lexical scope information generally.
Removing this need for the parser here will allow us to remove
`Interpreter::getParser` which we should not expose as an API at that stage.
================
Comment at: clang/lib/Interpreter/ValuePrinter.cpp:301
+
+ auto AddrOrErr = Interp.CompileDecl(WrapperFD);
+ if (!AddrOrErr)
----------------
I'd prefer to inline to body of `CompileDecl` here instead of exposing it in
the interpreter class.
================
Comment at: clang/tools/clang-repl/CMakeLists.txt:40
+ ??6?$basic_ostream@DU?$char_traits@D@std@@@std@@QEAAAEAV01@PEBX@Z
+
??6?$basic_ostream@DU?$char_traits@D@std@@@std@@QEAAAEAV01@P6AAEAV01@AEAV01@@Z@Z
+
??$?6U?$char_traits@D@std@@@std@@YAAEAV?$basic_ostream@DU?$char_traits@D@std@@@0@AEAV10@D@Z
----------------
We should check if there is a better way to export a subset of symbols through
the llvm build system. Maybe @lhames, @sunho or @sgraenitz know how.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146809/new/
https://reviews.llvm.org/D146809
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
