[PATCH] D155878: [clangd] Loose include-cleaner matching for verbatim headers

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/Headers.cpp:185
+  // The entries will be the same, but canonicalizing to find out is expensive!
+  if (SearchPathCanonical.empty()) {
+    for (const auto &Dir :
----------------
nit: early exit


================
Comment at: clang-tools-extra/clangd/Headers.h:179
+  // All paths are canonical (FileManager::getCanonicalPath()).
+  std::vector<std::string> SearchPathCanonical;
+
----------------
s/SearchPathCanonical/SearchPathsCanonical/


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1250
   const SourceManager &SM = AST.getSourceManager();
-  const auto &ConvertedMainFileIncludes =
-      convertIncludes(SM, AST.getIncludeStructure().MainFileIncludes);
-  const auto &HoveredInclude = convertIncludes(SM, llvm::ArrayRef{Inc});
+  const auto &Converted = convertIncludes(AST);
   llvm::DenseSet<include_cleaner::Symbol> UsedSymbols;
----------------
oops, looks like we were getting away with some dangling references.
the reference here is wrong, `convertIncludes` returns a value type. can you fix that while here?


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:486-491
+  include_cleaner::walkUsed(AST.getLocalTopLevelDecls(), {},
+                            AST.getPragmaIncludes(), AST.getSourceManager(),
+                            [&](const include_cleaner::SymbolReference &Ref,
+                                llvm::ArrayRef<include_cleaner::Header> P) {
+                              Providers[llvm::to_string(Ref.Target)] = P.vec();
+                            });
----------------
i don't think making this test rely on `walkUsed` is a good idea. what about just populating the providers directly, like:

```
EXPECT_TRUE(isPreferredProvider(Decl, Includes, {include_cleaner::Header{"decl.h"}, include_cleaner::Header{"def.h"}}));
```

and verifying we're recognizing preferred providers purely on that ordering ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155878