[PATCH] D61077: [clangd] Query index in code completion no-compile mode.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 3 inline comments as done.
sammccall added inline comments.


================
Comment at: clangd/SourceCode.cpp:503
+    case tok::l_brace:
+      if (State == NamespaceName) {
+        // Parsed: namespace <name> {
----------------
kadircet wrote:
> 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.
Right, this was intended. Added a comment and a test.


================
Comment at: clangd/SourceCode.cpp:595
+  });
+  Found.erase(std::unique(Found.begin(), Found.end()), Found.end());
+  return Found;
----------------
kadircet wrote:
> `scopesForIndexQuery` already de-duplicates. Do you plan to have any other users for the results of this function?
Only unit tests.
Seems a bit neater to always return canonical results, though.


================
Comment at: clangd/SourceCode.h:169
+/// The returned vector is always non-empty.
+/// - The first element is the namespace that encloses the point: a declaration
+///   near the point would be within this namespace.
----------------
kadircet wrote:
> Does the code ever make use of it?
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.


================
Comment at: unittests/clangd/SourceCodeTests.cpp:325
 
+TEST(SourceCodeTests, VisibleNamespaces) {
+  std::vector<std::pair<const char *, std::vector<std::string>>> Cases = {
----------------
kadircet wrote:
> NIT: maybe switch to TEST_P ?
I find TEST_P much less readable and prefer to avoid it unless absolutely necessary.
Does it buy anything here?(


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61077/new/

https://reviews.llvm.org/D61077