[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:97
+
+  enum InterfaceKind { NoAlloc, WithAlloc, CopyArray };
+
----------------
v.g.vassilev wrote:
> junaire wrote:
> > v.g.vassilev wrote:
> > > This can probably go in the RuntimeInterfaceBuilder class.
> > We need it. See:
> > 
> > ```
> > class RuntimeInterfaceBuilder
> >     : public TypeVisitor<RuntimeInterfaceBuilder, Interpreter::InterfaceKind> {
> >    ...
> > }
> > ```
> Can't this be an enum which is file local?
You can't put this enum inside RuntimeInterfaceBuilder because its declaration needs it. If you do so, then the above line will report an error since you use the enum before defined it.


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:350
   std::list<PartialTranslationUnit> &PTUs = IncrParser->getPTUs();
-  if (N > PTUs.size())
+  if (N + InitPTUSize > PTUs.size())
     return llvm::make_error<llvm::StringError>("Operation failed. "
----------------
v.g.vassilev wrote:
> junaire wrote:
> > v.g.vassilev wrote:
> > > v.g.vassilev wrote:
> > > > I'd propose `IncrParser->getPTUs()` to return the list starting from `InitPTUSize`. That should solve the issue you see.
> > > Ping.
> > I'm uncertain about how to do this. Can you elaborate?
> The `std::list<PartialTranslationUnit> PTUs;` in `IncrementalParser.h` stores the all PTUs that we saw, including the Interpreter builtins which contain declarations required by the Interpreter to run. I'd propose  to make `PTUs` of type `std::vector` and `getPTUs()` to return an `ArrayRef` starting from `PTUs.begin() + N` where `N` is the number of builtins that we must not undo.
OK, return ArrayRef sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141215