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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
