[PATCH] D42071: [Sema] Add visited contexts to CodeCompleteContext

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM.
Look at the comments for a few NITs.



================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:271
 
+  using VisitedContextSet = llvm::SmallPtrSet<DeclContext*, 8>;
+
----------------
Given that lookup does not visit the same `DeclContext` twice, we're probably fine with vector here, rather than the set.
On the other hand, using set gives a better understanding that the order of items is not specified.
I don't think we should necessarily changes the code here, just something to think about.



================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:290
 
+  /// \breif The declaration contexts Sema has already visited during code
+  /// completion.
----------------
s/breif/brief


================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:290
 
+  /// \breif The declaration contexts Sema has already visited during code
+  /// completion.
----------------
ilya-biryukov wrote:
> s/breif/brief
I'd mention lookup explicitly in the comment, e.g.
```
/// \brief A set of decl contexts visited when doing lookup for code completion.
```


================
Comment at: unittests/Sema/CodeCompleteTest.cpp:53
+    for (const auto *Context : VisitedContexts)
+      if (const auto *NS = llvm::dyn_cast<NamespaceDecl>(Context))
+        NSNames.push_back(NS->getQualifiedNameAsString());
----------------
Why onyl namespaces and not all `DeclContext`s?


================
Comment at: unittests/Sema/CodeCompleteTest.cpp:70
+  bool BeginInvocation(CompilerInstance &CI) override {
+    auto& opts = CI.getFrontendOpts();
+    opts.CodeCompletionAt = CompletePosition;
----------------
NIT: local var starts with a lower-case letter.


Repository:
  rC Clang

https://reviews.llvm.org/D42071