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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
