[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added inline comments.


================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:184
+  SourceLocation NewLoc = SM.getLocForStartOfFile(SM.getMainFileID())
+                              .getLocWithOffset(InputCount + 2);
+
----------------
The `+ 2` here is probably not what you want. This will just give you a pointer into Clang's source buffers and will eventually point to random source buffers (or worse) once InputCount is large enough.

I feel like the proper solution is to just use the StartOfFile Loc and don't add any offset to it. I think Clang should still be able to figure out a reasonable ordering for overload candidates etc. 

(I thought I already commented on this line before, but I can't see my comment or any replies on Phab so I'm just commenting again).


================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:63
+    if (CI.hasCodeCompletionConsumer())
+      CompletionConsumer = &CI.getCodeCompletionConsumer();
+
----------------
v.g.vassilev wrote:
> teemperor wrote:
> > Can this completion code even be used? It doesn't look like it can (and I'm not sure if using the `CodeCompletionAt` flag is really useful in a REPL as you can only specify it once during startup). IMHO this can be left out until we actually can hook up this into the LineEditor (and we have a way to test this).
> Cling, on which this patch is based on, has a CodeCompleteConsumer and it seems to work.
> 
> I can leave it out but then we will have to remember where to put it. I have a slight preference to leave it as it is.
Alright, given that this is just passing along the CompletionConsumer from Ci to Sema, I think this can stay then.


================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:189
+  // NewLoc only used for diags.
+  PP.EnterSourceFile(FID, /*DirLookup*/ 0, NewLoc);
+
----------------
v.g.vassilev wrote:
> teemperor wrote:
> > I think we could assert that this succeeds.
> assert on the FID you mean?
I meant the `EnterSourceFile` call has a `bool` error it returns (along with a diagnostic). I think the only way it should fail is if the previous code got somehow messed up, hence the assert suggestion.


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:82
+  PCHOps->registerWriter(std::make_unique<ObjectFilePCHContainerWriter>());
+  PCHOps->registerReader(std::make_unique<ObjectFilePCHContainerReader>());
+
----------------
v.g.vassilev wrote:
> teemperor wrote:
> > Can you add a FIXME that this should be removed (as I think we keep adding these two lines to all kind of Clang tools)? I assume this is only needed because of Clang errors when you encounter -gmodules produced pcms ?
> I do not understand the problem entirely, could you propose wording for the FIXME?
The PCM files generated with `-gmodules` are object-files that contain the Clang AST inside them. To deal with the object-file wrapping and get to the AST inside, we need the `ObjectFilePCHContainerReader`. But that class is part of CodeGen which isn't a dependency of the normal parsing logic, so these classes can't be hooked up automatically by Clang like the other 'ContainerReaders' classes (well, there is only one other classes which just opens the file normally).

So to work around the fact that to **parse** code we now need a **CodeGen** dependency, every clang tool (which usually depend on CodeGen + parsing code) has to manually register the `ObjectFilePCHContainer*` classes.

My point is just that it's not sustainable to have everyone copy around these three lines as otherwise Clang won't handle any PCM file that was generated with -gmodules.

I think the FIXME for this could be:
`FIXME: Clang should register these container operations automatically.`.


================
Comment at: clang/tools/clang-repl/CMakeLists.txt:3
+#  ${LLVM_TARGETS_TO_BUILD}
+#  Option
+  Support
----------------
v.g.vassilev wrote:
> teemperor wrote:
> > v.g.vassilev wrote:
> > > teemperor wrote:
> > > > Commented out by mistake?
> > > Does not seem to be required. I will remove it for now...
> > I meant that shouldn't have been commented out (as I think we don't get the targets symbols from any other library so this fails to link)
> It compiles just fine for me. Should we add it back once we find the setup that requires it?
On my Linux setup the dependencies were required (and it seems logical that they are required), so I would just add them.


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

https://reviews.llvm.org/D96033