[PATCH] D49012: [clangd] Uprank delcarations when "using q::name" is present in the main file

[Sam McCall via Phabricator via cfe-commits]

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