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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:271
 
+  using VisitedContextSet = llvm::SmallPtrSet<DeclContext*, 8>;
+
----------------
ilya-biryukov wrote:
> 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.
> 
Good point. Yeah, technically, there should be no duplicated DeclContext.


================
Comment at: include/clang/Sema/Lookup.h:791
+  /// \param Ctx the context which Sema begins to visit.
+  virtual void BeginVisitContext(DeclContext *Ctx) {};
 };
----------------
bkramer wrote:
> ilya-biryukov wrote:
> > Maybe rename it to `VisitedContext` ? Seems more in-line with `FoundDecl`.
> > `BeginVisitContext` also suggest there should be `EndVisitContext`
> Semicolon is not needed here.
Renamed it to `EnteredContext` based on offline discussion.


================
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());
----------------
ilya-biryukov wrote:
> Why onyl namespaces and not all `DeclContext`s?
Yeah, this is intended (the method name has indicated it), we only verify the results for namespaces in the test (for simplicity).


Repository:
  rC Clang

https://reviews.llvm.org/D42071