sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/Headers.cpp:39
+ if (!isInsideMainFile(HashLoc, SM)) {
+ auto PreLoc = SM.getPresumedLoc(HashLoc);
+ if (auto FE = SM.getFileManager().getFile(PreLoc.getFilename())) {
----------------
Is it possible that doing this for every include we process in the preamble
might be significantly expensive?
You could consider doing something like:
```
FileID BuiltinFile;
bool InBuiltin;
void FileChanged(Loc, Reason, _, PrevFID) override {
if (Reason == EnterFile && !BuiltinFile && SM.isWrittenInBuiltInFile(Loc)) {
BuiltinFile = SM.getFileID(Loc);
InBuiltin = true;
} else if (Reason == ExitFile && InBuiltinFile && BuiltinFile == PrevFID) {
InBuiltin = false;
}
}
```
and then only run this for `InBuiltin`. (In practice fairly soon during
processing, BuiltinFile will be set and InBuiltin will have become false, so
all of this will be short-circuited)
================
Comment at: clang-tools-extra/clangd/Headers.cpp:40
+ auto PreLoc = SM.getPresumedLoc(HashLoc);
+ if (auto FE = SM.getFileManager().getFile(PreLoc.getFilename())) {
+ if (SM.getFileEntryForID(SM.getMainFileID()) == *FE) {
----------------
wouldn't be SM.getFileEntryForID(PreLoc.FID) be more efficient?
================
Comment at: clang-tools-extra/clangd/Headers.cpp:42
+ if (SM.getFileEntryForID(SM.getMainFileID()) == *FE) {
+ HashLoc = SM.translateLineCol(SM.getMainFileID(), PreLoc.getLine(),
+ PreLoc.getColumn());
----------------
could use a comment to explain what's going on at this point: we're
transforming the parameters so we can process this as if it occurred in the
main file.
But actually it might be clearer to extract a function that both cases can call.
================
Comment at: clang-tools-extra/clangd/Headers.cpp:44
+ PreLoc.getColumn());
+ PreLoc = SM.getPresumedLoc(FilenameRange.getBegin());
+ auto FileNameBegin = SM.translateLineCol(
----------------
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();
}
```
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
