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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, Just readability stuff.



================
Comment at: clang-tools-extra/clangd/Headers.cpp:39
                           SrcMgr::CharacteristicKind FileKind) override {
-    if (isInsideMainFile(HashLoc, SM)) {
+    auto AddMainFileInc = [&](SourceLocation HashLoc) {
       Out->MainFileIncludes.emplace_back();
----------------
nit: Inc -> Include


================
Comment at: clang-tools-extra/clangd/Headers.cpp:39
                           SrcMgr::CharacteristicKind FileKind) override {
-    if (isInsideMainFile(HashLoc, SM)) {
+    auto AddMainFileInc = [&](SourceLocation HashLoc) {
       Out->MainFileIncludes.emplace_back();
----------------
sammccall wrote:
> nit: Inc -> Include
this structure is not terrible but the reverse-chronological-order hurts readability a bit.

You could try:
```
// If an include is part of the preamble patch, translate #line directives.
if (BuiltinFileInStack) {
  auto PreLoc = ...;
  if (...) {
    HashLoc = ...; // now we'll hit the case below
  }
}
// Record main-file inclusions (including those mapped from the preamble patch).
if (isInsideMainFile(HashLoc, SM)) {
  // add the include
}
```


================
Comment at: clang-tools-extra/clangd/Headers.cpp:55
+    if (BuiltinFileInStack) {
+      // Directives included through builtin file can be mapped back to main
+      // file via line directives.
----------------
can -> might, line -> #line, ... as part of a preamble patch
(just to hint a little more clearly who created the mapping)


================
Comment at: clang-tools-extra/clangd/Headers.cpp:57
+      // file via line directives.
+      auto PreLoc = SM.getPresumedLoc(HashLoc);
+      if (auto FE = SM.getFileManager().getFile(PreLoc.getFilename())) {
----------------
PreLoc -> Presumed?
(these abbreviations aren't common)


================
Comment at: clang-tools-extra/clangd/Headers.cpp:58
+      auto PreLoc = SM.getPresumedLoc(HashLoc);
+      if (auto FE = SM.getFileManager().getFile(PreLoc.getFilename())) {
+        if (SM.getFileEntryForID(MainFID) == *FE) {
----------------
For clarity I think we could do this only with an invalid file ID and pull out a function, ending up with:

```
if (Presumed.getFileID().isInvalid() && isMainFile(Presumed.getFilename()))
  AddMainFileInc(HashLoc);
```
  


================
Comment at: clang-tools-extra/clangd/Headers.cpp:65
+      }
+    } else if (isInsideMainFile(HashLoc, SM)) {
+      AddMainFileInc(HashLoc);
----------------
nit: I'd find it slightly clearer to handle this case first in the if/else, as it kind of "motivates" the more complicated one.


================
Comment at: clang-tools-extra/clangd/Headers.cpp:69
+
     if (File) {
       auto *IncludingFileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc));
----------------
maybe delimit this with a comment like
// Record include graph (not just for main-file includes)


================
Comment at: clang-tools-extra/clangd/Headers.cpp:107
+  // Indicates whether <built-in> file is part of include stack.
+  bool BuiltinFileInStack = false;
+
----------------
Using the name `stack` for a boolean was pretty confusing to me. I think `InBuiltinFile` plus the comment would be enough, otherwise maybe `UnderBuiltinFile` or so?


================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:309
+TEST_F(HeadersTest, PresumedLocations) {
+  std::string HeaderFile = testPath("implicit_include.h");
+
----------------
I don't think testPath is needed here: it should be on the include path, and FS.Files adds the qualifiers where needed


================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:314
+  HeaderContents += R"cpp(
+#line 1
+#include <a.h>)cpp";
----------------
Can we use more aribtrary placeholder here, like 42? :)
We could easily hit 1 by mistake.


================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:324
+  // Now include through built-in file.
+  CDB.ExtraClangFlags = {"-include" + HeaderFile};
+  EXPECT_THAT(collectIncludes().MainFileIncludes,
----------------
changing plus to comma here seems clearer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78740