[PATCH] D141215: [clang-repl] Introduce Value to capture expression results

[Jun Zhang via Phabricator via cfe-commits]

junaire marked 16 inline comments as done.
junaire added inline comments.


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:72
+  llvm::Error ParseAndExecute(llvm::StringRef Code, Value *V = nullptr);
+  llvm::Expected<llvm::JITTargetAddress> CompileDtorCall(CXXRecordDecl *CXXRD);
 
----------------
aaron.ballman wrote:
> Hopefully this function isn't intending to mutate the passed object?
Unfortunately `Sema::LookupDestructor` ask for a non-const pointer... (https://github.com/llvm/llvm-project/blob/26e164fada3721b0939e6b7c2dfe1793d95229bb/clang/lib/Sema/SemaLookup.cpp#L3625)


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:102
+
+  Expr *SynthesizeExpr(clang::Expr *E);
+
----------------
aaron.ballman wrote:
> Any chance we can make this const correct as well? (I'll stop asking in this review -- can you take a pass through it to add const correctness where it isn't obnoxiously viral to do so?)
FindRuntimeInterface mutates ValuePrintingInfo vector so it can't be const afaict.


================
Comment at: clang/include/clang/Interpreter/Value.h:17
+
+#include <cstdint>
+
----------------
aaron.ballman wrote:
> Unused include can be removed.
uintptr_t comes from this header so it's not unused.


================
Comment at: clang/include/clang/Interpreter/Value.h:77
+
+        K_Void,
+    K_PtrOrObj,
----------------
aaron.ballman wrote:
> Fixing indentation
It's already formatted by clang-format and if I change this arc doesn't even allow me to upload the diff. I believe it's a bug in clang-format and I have filed https://github.com/llvm/llvm-project/issues/60535 So I'd like to fix this when I commit this :)


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:534
+    }
+      llvm_unreachable("Unknown runtime interface kind");
+    }
----------------
sgraenitz wrote:
> Is this a leftover from an old `default` label?
there's no default label since we handle all the cases here. But indeed it looks a bit ugly and I removed it.


================
Comment at: clang/lib/Interpreter/Value.cpp:91
+  static constexpr unsigned char Canary[8] = {0x4c, 0x37, 0xad, 0x8f,
+                                              0x2d, 0x23, 0x95, 0x91};
+};
----------------
sgraenitz wrote:
> Can we add a comment with an explanation of the magic numbers please?
I'm uncertain if these numbers actually have specific meanings, I just copied them from Cling directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141215