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

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added inline comments.


================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:184
+  SourceLocation NewLoc = SM.getLocForStartOfFile(SM.getMainFileID())
+                              .getLocWithOffset(InputCount + 2);
+
----------------
teemperor wrote:
> 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).
> 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.

Indeed.

> 
> 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. 

That particular part of the input processing has been causing a lot of troubles for cling over the years. If we use the StartOfFile each new line will appear before the previous which can be problematic for as you say diagnostics but also template instantiations.

Cling ended up solving this by introducing a virtual file with impossible to allocate size of `1U << 15U` (https://github.com/vgvassilev/cling/blob/da1bb78f3dea4d2bf19b383aeb1872e9f2b117ad/lib/Interpreter/CIFactory.cpp#L1516-L1527 and https://github.com/vgvassilev/cling/blob/da1bb78f3dea4d2bf19b383aeb1872e9f2b117ad/lib/Interpreter/IncrementalParser.cpp#L394) Then we are save to get Loc + 1 (I do not remember how that + 2 came actually) and you should be still safe.

I wonder if that's something we should do here? 

> 
> (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();
+
----------------
teemperor wrote:
> 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.
Cool, thanks!


================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:189
+  // NewLoc only used for diags.
+  PP.EnterSourceFile(FID, /*DirLookup*/ 0, NewLoc);
+
----------------
teemperor wrote:
> 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.
Got it now, somehow overlooked that `EnterSourceFile` returns true on a failure. I decided to return an error.


================
Comment at: clang/tools/clang-repl/CMakeLists.txt:3
+#  ${LLVM_TARGETS_TO_BUILD}
+#  Option
+  Support
----------------
teemperor wrote:
> 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.
Ok, readded.


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

https://reviews.llvm.org/D96033