kadircet marked 5 inline comments as done.
kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/Headers.cpp:44
+ PreLoc.getColumn());
+ PreLoc = SM.getPresumedLoc(FilenameRange.getBegin());
+ auto FileNameBegin = SM.translateLineCol(
----------------
kadircet wrote:
> sammccall wrote:
> > This part looks a little iffy to me, with all the coordinate transforms.
> >
> > If we're synthesizing the include, chars don't have to match 1:1 right?
> > e.g. if the original code was `# include /* foo */ "bar.h" // baz`
> > and we synthesize `#include "bar.h"`, how is this going to get the
> > coordinates of "bar.h" right?
> >
> > This seems awkward to resolve. `R` isn't actually used much though,
> > go-to-definition looks at its line number only, and DocumentLink uses it
> > (but it seems OK to just to do approximate re-lexing there). Maybe we can
> > just drop it?
> >
> > ---
> > (Original comment disregarding above problem)
> >
> > Isn't it the case that the filename expansion location has to be in the
> > same file as the hashloc?
> > So can we do something like:
> >
> > ```
> > FilenameRange = SM.getExpansionRange(FilenameRange);
> > if (SM.getFileID(FilenameRange.start()) ==
> > SM.getFileID(FilenameRange.end()) == SM.getFileID(OrigHashLoc)) {
> > // all in the same file
> > // compute NewStart = OrigStart - OrigHashLoc + NewHashLoc, etc
> > } else {
> > FilenameRange = CharSourceRange();
> > }
> > ```
> > This part looks a little iffy to me, with all the coordinate transforms.
> >
> > If we're synthesizing the include, chars don't have to match 1:1 right?
> > e.g. if the original code was # include /* foo */ "bar.h" // baz and
> > we synthesize #include "bar.h", how is this going to get the coordinates of
> > "bar.h" right?
>
> well, the patching actually ensures both `#` and `"filename"` starts at the
> correct offset, by padding them with whitespaces ignoring any comments and
> such.
>
> >
> > This seems awkward to resolve. R isn't actually used much though,
> > go-to-definition looks at its line number only, and DocumentLink uses it
> > (but it seems OK to just to do approximate re-lexing there). Maybe we can
> > just drop it?
>
> I am fine with dropping it too, the padding looks really ugly in the patching
> code :D.
>
> Regarding go-to-def, I suppose we can keep storing the include line, since we
> calculate it anyway while getting the presumed location for HashLoc.
>
> For DocumentLink, I suppose we can either lex while handling the request or
> store those separately in parsedast. I would go with the former.
>
> WDYT?
sent out D79315.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78740/new/
https://reviews.llvm.org/D78740
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
