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