[clangd] Remove redundant emptiness check in cross ref calculation for includes.

[usaxena95]

clang-tools-extra/clangd/XRefs.cpp

1361–1362

Sorry for the confusion. This looks intentional and somewhat valuable for unused headers. We could fix this on clang-review's end as discussed on the internal patch. We should also add a comment here explaining the rationale behind returning no refs in such cases.

(The check below certainly looks redundant though and could be removed).

[kadircet]

clang-tools-extra/clangd/XRefs.cpp

1365

i guess this patch is not needed anymore, but one comment about this behaviour in clangd as well (no action needed in this patch)

why are we providing the reference to the include line itself? in other interactions we do that for the sake of completeness (e.g. if you trigger xrefs on a reference, seeing the declaration/definition location is useful) but usually the result under the cursor is not so interesting, especially in this case. the include line itself is drastically different than the nature of other references, the user can only see the include, if they triggered the xrefs on the include line itself and not when they trigger it on the symbol. also that ref doesn't count if there's no symbols used?

FWIW I am not sure if that's a useful interaction. What was the rationale here exactly? It might be useful to always emit this reference, but also append provider to the refs list in the other direction as well (that way the results will be coherent again)

[VitaNuo]

Remove only one of the checks.

[VitaNuo]

clang-tools-extra/clangd/XRefs.cpp

1361–1362

Yeah you're right the second check is redundant (probably an artefact of some unfinished refactoring). I will land this patch with the redundant check removed.

1365

I think the rationale was that it provides more clarity in the VS Code UI. If you look back at the prototype image in the internal design doc, the references list always looks like a dropdown list with the containing file (in this case main file only) as title. In case of normal references, it shows the same symbol in different contexts, and it makes sense intuitively. In case of includes, it looks like a forest of unconnected symbols and might look like a bug to a user. IIRC that's the reason we initially agreed to add clarity by adding the include line itself.

clang-tools-extra/clangd/XRefs.cpp

include_cleaner::walkUsed(
 
         ReferencesResult::Reference Result;
         const auto *Token = AST.getTokens().spelledTokenAt(Loc);
         Result.Loc.range = Range{sourceLocToPosition(SM, Token->location()),
                                  sourceLocToPosition(SM, Token->endLocation())};
         Result.Loc.uri = URIMainFile;
         Results.References.push_back(std::move(Result));
       });
   if (Results.References.empty())
     return std::nullopt;
 
   // Add the #include line to the references list.
   ReferencesResult::Reference Result;
   Result.Loc.range =
       rangeTillEOL(SM.getBufferData(SM.getMainFileID()), Inc.HashOffset);
   Result.Loc.uri = URIMainFile;
   Results.References.push_back(std::move(Result));
-
-  if (Results.References.empty())
-    return std::nullopt;
   return Results;
 }
 } // namespace
 
 ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
                                 const SymbolIndex *Index, bool AddContext) {
   ReferencesResult Results;
   const SourceManager &SM = AST.getSourceManager();