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

[Jun Zhang via Phabricator via cfe-commits]

junaire added inline comments.


================
Comment at: clang/lib/Interpreter/ValuePrinter.cpp:262
+
+static llvm::Expected<llvm::orc::ExecutorAddr> CompileDecl(Interpreter &Interp,
+                                                           Decl *D) {
----------------
v.g.vassilev wrote:
> 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.
I can't really get the point. I agree we shouldn't expose `Interpreter::GenModule` as a public interface but I don't understand the solution.


================
Comment at: clang/lib/Interpreter/ValuePrinter.cpp:442
+
+REPL_EXTERNAL_VISIBILITY std::string PrintValueRuntime(const char *const *Val) {
+  return PrintString(Val);
----------------
v.g.vassilev wrote:
> 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.
* I'd propose using `PrintValueRuntime` here. This is because we have used it across RFC and other documentation so I don't want to change it :(

> 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.

* That really touches the dark corner of our design. On the one hand, we try to keep `Value` itself as lightweight as possible, on the other hand, we use heavy headers like `<string>` and `<type_traits>` in value pretty printing. While this is awkward, we can hardly avoid it because the implementation needs to see the definition of these types to know how to print it. So how can we do this on resources limited systems? My point is that perhaps we can keep it as is, because it's very hard to measure how expensive the feature is on the targeting platform. If the system can afford this, then we don't need to worry about anything, but if the system can't even use STL itself, presumably it shouldn't use value printing at all! Note that this doesn't conflicts with the lightness of the value runtime as it SHOULD add as little overhead as possible no matter whether the host can afford it or not.


================
Comment at: clang/lib/Interpreter/ValuePrinter.cpp:473
+  else if (auto *BT = DesugaredTy.getCanonicalType()->getAs<BuiltinType>()) {
+    switch (BT->getKind()) {
+    case BuiltinType::Bool: {
----------------
v.g.vassilev wrote:
> We could use the preprocessor the way we do in `Value.cpp` to expand this dispatcher.
We actually can't because it doesn't strictly match everything. For example `Char_S` and `SChar` both map to `V.getSChar()` IIUC?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146809