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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits