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

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Sorry for the delay. I think all my points have been resolved beside the insertion SourceLoc.



================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:184
+  SourceLocation NewLoc = SM.getLocForStartOfFile(SM.getMainFileID())
+                              .getLocWithOffset(InputCount + 2);
+
----------------
v.g.vassilev wrote:
> 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).
> 
> 
I think my point then is: If we would change Clang's behaviour to consider the insertion order in the include tree when deciding the SourceLocation order, wouldn't that fix the problem? IIUC that's enough to make this case work the intended way without requiring some fake large source file. It would also make this work in other projects such as LLDB.

So IMHO we could just use StartOfFile as the loc here and then consider the wrong ordering as a Clang bug (and add a FIXME for that here).


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

https://reviews.llvm.org/D96033