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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo accepted this revision.
VitaNuo added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1361-1362
       });
-  if (Results.References.empty())
-    return std::nullopt;
-
----------------
usaxena95 wrote:
> 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).
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.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1365
   // Add the #include line to the references list.
   ReferencesResult::Reference Result;
   Result.Loc.range =
----------------
kadircet wrote:
> 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)
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155614