[PATCH] D78740: [clangd] Handle PresumedLocations in IncludeCollector

[Sam McCall via Phabricator via cfe-commits]

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