[PATCH] D147044: [clangd] Implement cross reference request for #include lines.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1338
+
+    auto ReferencedInclude = convertIncludes(SM, Inc);
+    include_cleaner::walkUsed(
----------------
can we put the rest into a separate function (I know this function is already quite long, but I think we shouldn't make it worse, and probably refactor soon). I believe something like below should work?
```
std::vector<Reference> getIncludeUsages(const ParsedAST &AST, const Inclusion &Include);
```


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1347
+
+          auto Loc = SM.getFileLoc(Ref.RefLocation);
+          for (const auto &H : Providers) {
----------------
nit: no need to calculate the file location, if we're not going to emit a reference. so maybe move this into the place where we're going to use the `Loc`.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1347
+
+          auto Loc = SM.getFileLoc(Ref.RefLocation);
+          for (const auto &H : Providers) {
----------------
kadircet wrote:
> nit: no need to calculate the file location, if we're not going to emit a reference. so maybe move this into the place where we're going to use the `Loc`.
this file location might fall outside of the main file when they get expanded through `#include` directives, so we need something like https://reviews.llvm.org/D147139


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1348
+          auto Loc = SM.getFileLoc(Ref.RefLocation);
+          for (const auto &H : Providers) {
+            auto MatchingIncludes = ConvertedMainFileIncludes.match(H);
----------------
we're implementing and testing this logic twice now, once in here and once in hover. can we instead have a helper in `IncludeCleaner.h` that looks like:
```
std::optional<include_cleaner::Header> firstSatisfiedProvider(const include_cleaner::Includes& Includes, llvm::ArrayRef<include_cleaner::Header> Providers);
// I'd actually return the matching `std::vector<const Include*>` (the highest ranked provider that matched some includes in main file), and check if the include of interest is part of that set for rest of the operations.
// Since it represents both the provider and the include in the main file. whereas the provider on it's own doesn't say anything about which include in main file triggered satisfaction.
```
and turn these call sites into
```
auto Provider = firstSatisfiedProvider(ConvertedMainFileIncludes, Providers);
if(!Provider || ReferencedInclude.match(Provider).empty())
  return;
// Include in question provides the symbol, do magic.
```


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1358
+              auto TokLen =
+                  Lexer::MeasureTokenLength(Loc, SM, AST.getLangOpts());
+              Result.Loc.range =
----------------
no need to re-run the lexer, these references are inside the main file for sure, so you can use token buffer instead.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1381
+    Result.Loc.uri = URIMainFile;
+    Results.References.push_back(std::move(Result));
+  }
----------------
we can return Results directly here, no need to run rest of the logic


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:2004
 
-// Given a type targeted by the cursor, return one or more types that are more interesting
-// to target.
-static void unwrapFindType(
-    QualType T, const HeuristicResolver* H, llvm::SmallVector<QualType>& Out) {
+// Given a type targeted by the cursor, return one or more types that are more
+// interesting to target.
----------------
can you revert changes below? I feel like they're unrelated formatting changes


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:1909
   TU.ExtraArgs.push_back("-std=c++20");
+  TU.AdditionalFiles["bar.h"] = guard(R"cpp(
+    #define BAR 5
----------------
rather than introducing these into all tests, can we have our own TestTU and whatnot in the relevant test? something like MainFileReferencesOnly test


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2322
+  const char *Tests[] = {
+      R"cpp([[#include ^"bar.h"]]
+        int fstBar = [[bar1]]();
----------------
can you also have a second include header that provides some other symbols and make sure references of those don't get highlighted?


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2329
+
+      R"cpp([[#in^clude <vector>]]
+        std::[[vector]]<int> vec;
----------------
we don't really need rest of these tests, they're testing the "symbol is provided by first provider included in the main file" logic


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147044