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

Reply via email to