sammccall added a comment. Nice! Mostly just a few nits.
================ Comment at: clang-tools-extra/unittests/clangd/QualityTests.cpp:83 + + namespace foo { + class Bar {}; ---------------- nit: can we have slightly more descriptive names here as this is a long test? e.g. `namespace hdr { class Used; }` (I think if you forward-declare the class, clang-format will let you have it on one line) ================ Comment at: clang-tools-extra/unittests/clangd/QualityTests.cpp:88 + namespace bar { + class Foo {}; + } // namespace bar ---------------- this bar::Foo proximity test doesn't seem to be substantially different from header(), could you drop it to reduce noise?( ================ Comment at: clang-tools-extra/unittests/clangd/QualityTests.cpp:122 EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f) - << "Current file and header"; + << "Current file and definition in header"; + ---------------- definition is not relevant here, AFAICS, revert this comment? (and in fact this is defined in the main file, not the header) ================ Comment at: clang-tools-extra/unittests/clangd/QualityTests.cpp:128 + UsingShadowDecl *Shadow; + auto findUsingDecl = [&SymbolName, &Shadow, + &FoundDecl](const NamedDecl &ND) -> bool { ---------------- nit: spelling out the captures and types here adds a lot of noise to the test and doesn't seem to gain much in return. What about inlining this, capturing `[&]`, and letting the return type be implicit? ================ Comment at: clang-tools-extra/unittests/clangd/QualityTests.cpp:131 + if (const UsingDecl *Using = dyn_cast<UsingDecl>(&ND)) { + if (UsingShadowDecl *ShadowDecl = *Using->shadow_begin()) { + if (ShadowDecl->getTargetDecl()->getName() == SymbolName) { ---------------- doesn't this crash if there are no shadows? ================ Comment at: clang-tools-extra/unittests/clangd/QualityTests.cpp:133 + if (ShadowDecl->getTargetDecl()->getName() == SymbolName) { + Shadow = ShadowDecl; + FoundDecl = Shadow->getTargetDecl(); ---------------- `findAnyDecl` is designed to return the decl you find, idiomatically the filter shouldn't have side effects. What about: ``` auto *Shadow = *findAnyDecl(AST, [&](const NamedDecl& ND) { if (... Using = ...) return Using->getQualifiedNameAsString() == "Bar" && Using->shadow_size(); return false; }).shadow_begin(); CodeCompletionResult Result(Shadow->getTargetDecl()); Result.setShadowDecl(Shadow); ``` ================ Comment at: clang-tools-extra/unittests/clangd/QualityTests.cpp:146 + EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f) + << "Uprank declaration which has using directive in current directory"; + ---------------- nit: "current directory" isn't relevant here, rather "main file"? nit: "uprank" isn't quite right here - ranking/scoring is a concern of extraction, rather evaluate(). Prefer just describing the situation ("Using declaration in main file") nit: these are using declarations, not using directives ================ Comment at: clang/include/clang/Sema/CodeCompleteConsumer.h:852 + std::vector<FixItHint> FixIts = std::vector<FixItHint>(), + const UsingShadowDecl *ShadowDecl = nullptr) : Declaration(Declaration), Priority(Priority), Kind(RK_Declaration), ---------------- we don't need both the constructor and the setter/public field - I think the constructor param is actually unused. Drop? ================ Comment at: clang/include/clang/Sema/CodeCompleteConsumer.h:911 + void setShadowDecl(const UsingShadowDecl *Shadow) { + this->ShadowDecl = Shadow; ---------------- the field is already public, no need to add a setter https://reviews.llvm.org/D49012 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits