ilya-biryukov added a comment.
Mostly NITs from my side, the change LG, thanks!
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:422
+ // "using namespace" declaration doesn't have a name.
+ Refs.push_back({D->getQualifierLoc(),
+ D->getIdentLocation(),
----------------
Same comment as below: could you say `ReferenceLoc` explicitly here and in
other places?
Plain initializer lists are somewhat hard to parse
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:436
+ // For namespace alias, "namespace Foo = Target;", we add two references.
+ VisitNamedDecl(D); // add a declaration reference for Foo.
+ // Add a non-declaration reference for Target.
----------------
NIT: maybe put this comment on a previous line?
Should read more naturally.
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:658
+ } else if (auto *E = N.get<Expr>()) {
+ if (auto Ref = refInExpr(E))
+ return {*Ref};
----------------
I'd probably suggest to return `SmallVector<ReferenceLoc, 2>` from all
functions, so that they compose better in this situation.
Although `Optional<ReferenceLoc>` captures the semantics of some of those
better, I don't think it's particularly useful at this point.
And having to deconstruct all optionals here leads to a lot of clunky code,
it'd be much better if we could keep the structure the same as it's used to be.
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:661
+ } else if (auto *NNSL = N.get<NestedNameSpecifierLoc>()) {
+ return {{NNSL->getPrefix(), NNSL->getLocalBeginLoc(), false,
+ explicitReferenceTargets(
----------------
Could we have `{ReferenceLoc{` instead of `{{`?
Plain initializer lists are hard to comprehend, especially when they're nested.
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:667
+ return {*Ref};
+ // return refInTypeLoc(*TL);
+ } else if (const CXXCtorInitializer *CCI = N.get<CXXCtorInitializer>()) {
----------------
Remove this comment? Seems to be a leftover from experiments
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:670
if (CCI->isBaseInitializer())
- return refInTypeLoc(CCI->getBaseClassLoc());
+ if (auto Ref = refInTypeLoc(CCI->getBaseClassLoc()))
+ return {*Ref};
----------------
NIT: add braces around an inner multi-line if?
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2280
}
- EXPECT_THAT(Names, UnorderedElementsAreArray(C.ExpectedDecls));
+ EXPECT_THAT(Names, UnorderedElementsAreArray(C.ExpectedDecls))
+ << File.code();
----------------
This looks unrelated. Maybe revert and land separately?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68977/new/
https://reviews.llvm.org/D68977
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
