Page MenuHomePhabricator

[clangd] Query index in code completion no-compile mode.
ClosedPublic

Authored by sammccall on Apr 24 2019, 10:02 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Apr 24 2019, 10:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2019, 10:02 AM
kadircet added inline comments.Apr 25 2019, 5:27 AM
clangd/SourceCode.cpp
503 ↗(On Diff #196476)

I believe it is safe to ignore(just mark the opening brace) anonymous namespaces here. Since there were no comments(and no test cases) just wanted to make sure you did not miss that case.

595 ↗(On Diff #196476)

scopesForIndexQuery already de-duplicates. Do you plan to have any other users for the results of this function?

clangd/SourceCode.h
169 ↗(On Diff #196476)

Does the code ever make use of it?

unittests/clangd/SourceCodeTests.cpp
325 ↗(On Diff #196476)

NIT: maybe switch to TEST_P ?

sammccall marked 2 inline comments as done.

Add comments, add anon-namespacce test, tighten parsing rules slightly (namespace ::... is illegal)

sammccall marked 3 inline comments as done.Apr 25 2019, 10:33 AM
sammccall added inline comments.
clangd/SourceCode.cpp
503 ↗(On Diff #196476)

Right, this was intended. Added a comment and a test.

595 ↗(On Diff #196476)

Only unit tests.
Seems a bit neater to always return canonical results, though.

clangd/SourceCode.h
169 ↗(On Diff #196476)

This is passed into the ScopeDistance, and the first scope gets a quality boost. Added a comment.

I just noticed that getQueryScopes (not used on this codepath) only sometimes returns the scopes in the right order. Will fix in another patch.

unittests/clangd/SourceCodeTests.cpp
325 ↗(On Diff #196476)

I find TEST_P much less readable and prefer to avoid it unless absolutely necessary.
Does it buy anything here?(

sammccall marked an inline comment as done.

Correctly handle absolutely qualifier (::foo::bar).
Fix seed scopes for proximity to be consistent with Sema case.

kadircet accepted this revision.Apr 26 2019, 12:40 AM

LGTM, thanks!

unittests/clangd/SourceCodeTests.cpp
325 ↗(On Diff #196476)

I just wanted to make sure we don't have more huge test cases as in Hover.All. I believe it would've helped if we've kept cases in small groups.

But I guess we won't gain much here, since number of cases is not huge and I think there won't be many additions.

This revision is now accepted and ready to land.Apr 26 2019, 12:40 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2019, 12:46 AM