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

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 17 06:03:54 PST 2018

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).

  rC Clang


More information about the cfe-commits mailing list