[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/IncludeCleaner.cpp:443
+  // No match for this provider in the includes list.
+  return {};
+}
----------------
`return std::nullopt`


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:87
+std::optional<include_cleaner::Header>
+firstMatchedProvider(const include_cleaner::Includes &Includes,
+                     llvm::ArrayRef<include_cleaner::Header> Providers);
----------------
can you also move tests related to this functionality from HoverTests into IncludeCleanerTests ?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1320
 
+ReferencesResult maybeFindIncludeReferences(ParsedAST &AST, Position Pos,
+                                            URIForFile URIMainFile) {
----------------
can you put this into anon namespace?

we should also change the return type to `std::optional<ReferencesResult>`, as we can "legitimately" have empty result for an include, e.g. it's unused or all of the refs are implicit etc.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1324
+  const SourceManager &SM = AST.getSourceManager();
+  auto Includes = AST.getIncludeStructure().MainFileIncludes;
+  auto ConvertedMainFileIncludes = convertIncludes(SM, Includes);
----------------
`const auto &Includes = AST.getIncludeStructure().MainFileIncludes;` (`&` is the important bit, as we're copying all of them otherwise)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1325
+  auto Includes = AST.getIncludeStructure().MainFileIncludes;
+  auto ConvertedMainFileIncludes = convertIncludes(SM, Includes);
+  for (auto &Inc : Includes) {
----------------
let's move this into the for loop, after we've found a matching include to not necessarily create a copy of all the includes again.

nit: you can also write the loop below as following and reduce some nesting.
```
auto IncludeOnLine = llvm::find_if(Includes, [&Pos](const Inclusion& Inc) { return Inc.HashLine == Pos.Line; });
if (IncludeOnLine == Includes.end())
  return Results;
const auto &Inc = *IncludeOnLine;
...
```


================
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);
----------------
VitaNuo wrote:
> kadircet wrote:
> > 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.
> > ```
> Is the comment under the code in the first snippet a mistake/outdated content? It confused me.
that wasn't supposed to be a comment **in** the code but rather a comment **for** the code :D i was suggesting to return the set of `Include`s matched by the first provider, rather than returning the provider. but feel free to keep it as-is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147044