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

[Jun Zhang via Phabricator via cfe-commits]

junaire added inline comments.


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:96
+
+  size_t getEffectivePTUSize() const;
+
----------------
aaron.ballman wrote:
> It looks like this can be private?
> 
> Also, just a note (not something you have to deal with in this review), the local naming convention seems to have decided that if the function starts with `get` then it's camel case and otherwise the name is pascal case.
Fixed.


================
Comment at: clang/include/clang/Interpreter/Value.h:83
+  Value() = default;
+  Value(void /*Interpreter*/ *In, void /*QualType*/ *Ty);
+  Value(const Value &RHS);
----------------
aaron.ballman wrote:
> v.g.vassilev wrote:
> > junaire wrote:
> > > aaron.ballman wrote:
> > > > Why do these take `void *` instead of the expected type?
> > > Yeah for the first parameter, we could just use `Interpreter*` but the second one is an opaque type so I think we should keep it?
> > See my previous comments on performance. We cannot include anything bulky in the header file.
> I think I understand why the design is the way it is, but it still makes me uneasy. The constructor takes a pointer to some bucket of bytes... no size information, no type information, etc. Just "here's a random pointer". And then later, we hope the user calls `setKind()` in a way that makes sense.
> 
> We need it to be fast, but we also need it to be correct -- the type system is the best tool for helping with that.
Not really... The user doesn't need to call `setKind()` explicitly to construct a `Value`, the constructor will handle it automatically. See `ConvertQualTypeToKind` in `Value.cpp`. So if the pointer is just some garbage data, the constructor should fail before yielding out a valid instance.


================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:22
 #include "clang/FrontendTool/Utils.h"
+#include "clang/Interpreter/Interpreter.h"
 #include "clang/Parse/Parser.h"
----------------
aaron.ballman wrote:
> v.g.vassilev wrote:
> > This introduces a layering violation. Can we avoid it?
> Does it? This file is in `clang/lib/Interpreter` so including other things from `clang/Interpreter` would not be a layering violation normally.
That's an old comment, already fixed and marked it as done :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141215