Page MenuHomePhabricator

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

Authored by omtcyfz on Jul 6 2018, 2:13 AM.

Details

Summary

Having using qualified::name; for some symbol is an important signal for clangd code completion as the user is more likely to use such symbol. This patch helps to uprank the relevant symbols by saving UsingShadowDecl in the new field of CodeCompletionResult and checking whether the corresponding UsingShadowDecl is located in the main file later in ClangD code completion routine. While the relative importance of such signal is a subject to change in the future, this patch simply bumps DeclProximity score to the value of 1.0 which should be enough for now.

The patch was tested using

$ ninja check-clang check-clang-tools

No unexpected failures were noticed after running the relevant testsets.

Diff Detail

Event Timeline

omtcyfz created this revision.Jul 6 2018, 2:13 AM
omtcyfz updated this revision to Diff 154377.Jul 6 2018, 2:16 AM

Remove comments from TestTU.cpp, both functions are properly documented in the corresponding header.

omtcyfz updated this revision to Diff 154379.Jul 6 2018, 2:44 AM

Add const qualifier to UsingShadowDecl in unit tests.

Nice! Mostly just a few nits.

clang-tools-extra/unittests/clangd/QualityTests.cpp
83 ↗(On Diff #154377)

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)

88 ↗(On Diff #154377)

this bar::Foo proximity test doesn't seem to be substantially different from header(), could you drop it to reduce noise?(

122 ↗(On Diff #154377)

definition is not relevant here, AFAICS, revert this comment?
(and in fact this is defined in the main file, not the header)

128 ↗(On Diff #154377)

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?

131 ↗(On Diff #154377)

doesn't this crash if there are no shadows?

133 ↗(On Diff #154377)

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);
146 ↗(On Diff #154377)

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

clang/include/clang/Sema/CodeCompleteConsumer.h
852 ↗(On Diff #154377)

we don't need both the constructor and the setter/public field - I think the constructor param is actually unused. Drop?

911 ↗(On Diff #154377)

the field is already public, no need to add a setter

ioeric added a comment.Jul 6 2018, 3:07 AM

Nice!

Just one caveat regarding locations in addition to Sam's comments :)

clang-tools-extra/clangd/Quality.cpp
48 ↗(On Diff #154379)

I think we might want to use getExpansionLoc here. Consider DEFINE_string which is implemented like:

#define DEFINE_string(XXX) \
namespace FLs { \
SomeType FLAGS_XXX; \
} \
using FLs::FLAGS_XXX;

When you have `DEFINE_string(X) in the main file, the spelling location will be in the file where the macro is defined, while the expansion location will be in the main file, and I think we would want later.

omtcyfz updated this revision to Diff 154383.Jul 6 2018, 3:34 AM
omtcyfz marked 10 inline comments as done.

Address the comments, add a couple of FIXMEs for the future.

sammccall accepted this revision.Jul 6 2018, 4:22 AM
sammccall added inline comments.
clang-tools-extra/clangd/Quality.cpp
48 ↗(On Diff #154379)

If you're not motivated to add the test now, I doubt anyone ever will be, so just remove the FIXME.

clang-tools-extra/unittests/clangd/QualityTests.cpp
116 ↗(On Diff #154383)

(this still says "definition", which doesn't seem true/relevant)

119 ↗(On Diff #154383)

nit: inline this?
(or if you really want to name it, give a more specific name)

This revision is now accepted and ready to land.Jul 6 2018, 4:22 AM
omtcyfz updated this revision to Diff 154956.Jul 11 2018, 3:25 AM
omtcyfz marked 3 inline comments as done.

Address few comments, add test for macro definition, slightly reformat QualityTests.cpp

omtcyfz updated this revision to Diff 154969.Jul 11 2018, 4:47 AM

Slightly refactor code by making lambda only used once anonymous.

This revision was automatically updated to reflect the committed changes.