[PATCH] D141215: [clang-repl] Introduce Value to capture expression results
Jun Zhang via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Apr 15 08:57:55 PDT 2023
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
More information about the cfe-commits
mailing list