[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:59
+
+  Value LastValue;
 
----------------
v.g.vassilev wrote:
> aaron.ballman wrote:
> > 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.
> One way to think of this is every `repl` has some way to access the last result it created when executed its last chunk of input. In python that's the `_` value. In the debugger it is the `%N` where N is some number. Here this variable would enable implementing a similar to these in future.
I added some comments.


================
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:
> junaire wrote:
> > 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.
> Yeah, that's a fair point, except nothing actually validates that the opaque pointer you are handed is actually valid for anything because it eventually just does a reinterpret_cast, so I don't think the constructor will fail.
OK, let me try to answer why we're passing `QualType` in an opaque pointer way. So the `Value` is *constructed* in `__clang_Interpreter_SetValue*` variants, and these functions (let's call them runtime interfaces) are synthesized when we want to do value printing (like we have a `x` without semi). So it makes things pretty hard if we use a complete type (How can we synthesize that type? That dramatically complicated the code) In addition, we want `Value.h`, which is part of the runtime, as lightweight as possible, so we can't use the complete type in its constructor, or we have to include the corresponding header file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141215