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