[PATCH] D58037: [clangd] Prefer location from codegen files when merging symbols.

[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: clangd/XRefs.cpp:261
 }
+// Returns the preferred location between an AST location and an index location.
+SymbolLocation getPreferredLocation(const Location &ASTLoc,
----------------
nit: move this and toIndexLocation together?


================
Comment at: clangd/XRefs.cpp:364
+        if (Sym.CanonicalDeclaration) {
+          // We only do this for declarations as definitions from AST
+          // is generally preferred (e.g. definitions in main file).
----------------
a little unclear what "we only do this" refers to - maybe missing a first line of the comment here.

// Use merge logic to choose AST or index declaration.


================
Comment at: clangd/index/Merge.cpp:155
+      getPreferredLocation(S.CanonicalDeclaration, O.CanonicalDeclaration);
+  S.Definition = getPreferredLocation(S.Definition, O.Definition);
   S.References += O.References;
----------------
I think it might be clearer what this is doing, and more similar with surrounding code, if the function returns a bool:

```
bool prefer(const SymbolLocation &Candidate, const SymbolLocation &Baseline);
...
if (!S.Definition || prefer(O.Definition, S.Definition))
  S.Definition = O.Definition;
```

up to you


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58037