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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I've not done a complete review yet, but I got started on it and have a handful of comments.



================
Comment at: clang/include/clang/Basic/TokenKinds.def:945
+// Annotation for end of input in clang-repl.
+ANNOTATION(input_end)
+
----------------
Should we name this `repl_input_end` to make it clear this is generally expected to only be used for REPL input?


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:59
+
+  Value LastValue;
 
----------------
I think I'm surprised to see this as a data member of `Interpreter` but mostly because my brain keeps thinking this interpreter is to interpret whole programs, so the idea of a "last value" is a bit odd from that context. The name is probably fine as-is, but I figured it may be worth mentioning just the same.


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:65
   create(std::unique_ptr<CompilerInstance> CI);
+  ASTContext &getASTContext() const;
   const CompilerInstance *getCompilerInstance() const;
----------------
Overload set for some better const correctness.


================
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);
 
----------------
Hopefully this function isn't intending to mutate the passed object?


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:98-100
+  llvm::SmallVectorImpl<Expr *> &getValuePrintingInfo() {
+    return ValuePrintingInfo;
+  }
----------------
`const` overload here as well.


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:102
+
+  Expr *SynthesizeExpr(clang::Expr *E);
+
----------------
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?)


================
Comment at: clang/include/clang/Interpreter/Value.h:17
+
+#include <cstdint>
+
----------------
Unused include can be removed.


================
Comment at: clang/include/clang/Interpreter/Value.h:43
+
+class Interpreter;
+class QualType;
----------------
Duplicates line 27, can be removed.


================
Comment at: clang/include/clang/Interpreter/Value.h:44
+class Interpreter;
+class QualType;
+
----------------
Move this up to the rest of the forward declarations (and probably keep them in alphabetical order).


================
Comment at: clang/include/clang/Interpreter/Value.h:46
+
+#define REPL_BUILTIN_TYPES                                                     \
+  X(bool, Bool)                                                                \
----------------
Is this expected to be a complete list of builtin types? e.g., should this have `char8_t` and `void` and `wchar_t`, etc? Should this be including `clang/include/clang/AST/BuiltinTypes.def` instead of manually maintaining the list?


================
Comment at: clang/include/clang/Interpreter/Value.h:77
+
+        K_Void,
+    K_PtrOrObj,
----------------
Fixing indentation


================
Comment at: clang/include/clang/Interpreter/Value.h:83
+  Value() = default;
+  Value(void /*Interpreter*/ *In, void /*QualType*/ *Ty);
+  Value(const Value &RHS);
----------------
Why do these take `void *` instead of the expected type?


================
Comment at: clang/include/clang/Interpreter/Value.h:121
+    static T cast(const Value &V) {
+      if (V.isPointerOrObjectType())
+        return (T)(uintptr_t)V.getAs<void *>();
----------------
Do we have to worry about member function pointers where a single pointer value may not be sufficient?


================
Comment at: clang/include/clang/Interpreter/Value.h:143
+  /// Values referencing an object are treated as pointers to the object.
+  template <typename T> T castAs() const { return CastFwd<T>::cast(*this); }
+
----------------
This doesn't match the usual pattern used in Clang where the `get` variant returns `nullptr` and the `cast` variant asserts if the cast would be invalid. Should we go with a similar approach?


================
Comment at: clang/include/clang/Interpreter/Value.h:160-162
+  // Interpreter, QualType are stored as void* to reduce dependencies.
+  void *Interp = nullptr;
+  void *OpaqueType = nullptr;
----------------
Why don't forward declares suffice if we're storing the information by pointer?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:7226
+  // assert(DeferredDeclsToEmit.empty() &&
+  //        "Should have emitted all decls deferred to emit.");
   assert(NewBuilder->DeferredDecls.empty() &&
----------------
v.g.vassilev wrote:
> That should probably be a separate review with a testcase.
+1 -- the codegen code owners should weigh in on whether this is reasonable as a temporary measure or not.


================
Comment at: clang/lib/Interpreter/CMakeLists.txt:14-16
   Interpreter.cpp
+  Value.cpp
+  InterpreterUtils.cpp
----------------
Probably should be kept alphabetical.


================
Comment at: clang/lib/Interpreter/IncrementalParser.h:34
 class Parser;
-
+class Interpreter;
 /// Provides support for incremental compilation. Keeps track of the state
----------------
Alphabetical order


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141215