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

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added inline comments.


================
Comment at: clang/lib/Interpreter/IncrementalParser.h:86
 
+  Parser &getParser() { return *P; }
+
----------------
Is that used?


================
Comment at: clang/lib/Interpreter/ValuePrinter.cpp:262
+
+static llvm::Expected<llvm::orc::ExecutorAddr> CompileDecl(Interpreter &Interp,
+                                                           Decl *D) {
----------------
Let's add a FIXME here. The problem `CompileDecl` and `GenModule` intends to solve is that when we synthesize AST we need to inform the rest of the Clang infrastructure about it and attach the produced `llvm::Module` to the JIT so that it can resolve symbols from it.

In cling that is solved with a RAII object which marks a scope where the synthesis happens and takes care to inform the rest of the infrastructure. We need something similar and a little more robust maybe. Something like `ASTSynthesisRAII` which ultimately hides this machinery especially from the public API.


================
Comment at: clang/lib/Interpreter/ValuePrinter.cpp:442
+
+REPL_EXTERNAL_VISIBILITY std::string PrintValueRuntime(const char *const *Val) {
+  return PrintString(Val);
----------------
All of the `PrintValueRuntime` seem to be duplicating code. We could use the preprocessor to expand these to but it would require some changes which I believe could be done as a separate patch:

* Perhaps we should be compatible with cling here in terms of naming as that's a public API - there I believe we use `printValue`.
* We should try to leverage `TemplateBase.cpp::printIntegral` for integral types.
* We should not return `std::string` here but a `const char*` and we should provide somewhere storage for them. That would probably make porting to embedded systems easier since on many the <string> header is hard to support. 

I believe it would be enough to have a fixme for now.


================
Comment at: clang/lib/Interpreter/ValuePrinter.cpp:473
+  else if (auto *BT = DesugaredTy.getCanonicalType()->getAs<BuiltinType>()) {
+    switch (BT->getKind()) {
+    case BuiltinType::Bool: {
----------------
We could use the preprocessor the way we do in `Value.cpp` to expand this dispatcher.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146809/new/

https://reviews.llvm.org/D146809