[PATCH] D50438: [clangd] Sort GoToDefinition results.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.
Herald added a subscriber: kadircet.


================
Comment at: clangd/XRefs.cpp:105
+    // Sort results. Declarations being referenced explicitly come first.
+    std::sort(Result.begin(), Result.end(),
+              [](const DeclInfo &L, const DeclInfo &R) {
----------------
ilya-biryukov wrote:
> Maybe sort further at the callers instead?
> It would be a more intrusive change, but would also give a well-defined result order for findDefinitions and other cases. E.g. findDefinitions currently gives results in an arbitrary order (specifically, the one supplied by DenseMap+sort) when multiple decls are returned.
> WDYT?
Just to clarify the original suggestion.

Maybe we can sort the `(location, is_explicit)` pairs instead of the `(decl, is_explicit)` pairs?
Sorting based on pointer equality (see `L.D < R.D`) provides non-deterministic results and we can have fully deterministic order on locations.

DeclarationAndMacrosFinder can return the results in arbitrary orders and the client code would be responsible for getting locations and finally sorting them.
WDYT?


================
Comment at: clangd/XRefs.cpp:296
   // Emit all symbol locations (declaration or definition) from AST.
-  for (const auto *D : Symbols.Decls) {
+  for (const auto &DI : Symbols.Decls) {
+    const auto *D = DI.D;
----------------
Maybe use explicit type here?  Using 'auto'  is a bit confusing.


================
Comment at: clangd/XRefs.cpp:297
+  for (const auto &DI : Symbols.Decls) {
+    const auto *D = DI.D;
     // Fake key for symbols don't have USR (no SymbolID).
----------------
Maybe use explicit type here too or rename the field from 'D' to something more descriptive (e.g. `Decl `)?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50438