[PATCH] D146809: [clang-repl] Implement Value pretty printing

[Vassil Vassilev via Phabricator via cfe-commits]

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 at DU?$char_traits at D@std@@@std@@QEAAAEAV01 at PEBX@Z
+      ??6?$basic_ostream at DU?$char_traits at D@std@@@std@@QEAAAEAV01 at P6AAEAV01@AEAV01@@Z at Z
+      ??$?6U?$char_traits at D@std@@@std@@YAAEAV?$basic_ostream at DU?$char_traits at D@std@@@0 at AEAV10@D at 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