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