[PATCH] D61126: [clangd] Also perform merging for symbol definitions

[Sam McCall via Phabricator via cfe-commits]

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


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:350
       if (R.Definition) { // from AST
+        // In case of generated files we prefer to omit the definition in the
+        // generated code.
----------------
Maybe move this below the PreferredDeclaration assignment?

It might just be me, but the generated code case seems much more obscure than the AST/index conflict case.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:350
       if (R.Definition) { // from AST
+        // In case of generated files we prefer to omit the definition in the
+        // generated code.
----------------
sammccall wrote:
> Maybe move this below the PreferredDeclaration assignment?
> 
> It might just be me, but the generated code case seems much more obscure than the AST/index conflict case.
The connection between the comment and the code seems a little obscure to me.
maybe. // We may still prefer the definition from the index, e.g. for generated symbols.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61126