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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Sorry, I just realized I didn't reply the comments in the first review (they were in my draft).



================
Comment at: clangd/XRefs.cpp:71
 
+struct DeclInfo {
+  const Decl *D;
----------------
ilya-biryukov wrote:
> NIT: maybe call `Occurence` instead? As this is actually a `Decl` with some extra data, computed based on the expression it originated from. Occurence seems like a nice that for that kind of thing.
I'm not sure what's a good name here, but I think we'd better avoid using `Occurrence`, because this term is used by `xrefs` (and we will have xrefs implementation in this file).


================
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:
> 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?
I think we'd better sort them in `DeclarationAndMacrosFinder` -- because we have a few clients in `XRefs.cpp` using this class, and it seems that they don't have their specific needs for sorting, having them sorting results seems unnecessary.


================
Comment at: clangd/XRefs.cpp:128
+  void insertDecl(const Decl *D, bool IsExplicitReferenced) {
+    auto It = Decls.find(D);
+    if (It != Decls.end())
----------------
ilya-biryukov wrote:
> Maybe simplify to `Decls[D] |= IsExplicitReferenced`?
Good point!


================
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).
----------------
ilya-biryukov wrote:
> Maybe use explicit type here too or rename the field from 'D' to something more descriptive (e.g. `Decl `)?
use explicit type here (I avoided using `Decl` as a variable name, because `Decl` is a type name in clang already).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50438