[PATCH] D69608: [clangd] Helper for getting nested namespace qualification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Just NITs about various comments and requests to optimize the code a bit.
The code itself LG, thanks!



================
Comment at: clang-tools-extra/clangd/AST.cpp:77
+llvm::DenseSet<const NamespaceDecl *>
+getUsingNamespceDirectives(const DeclContext *DestContext,
+                           SourceLocation Until) {
----------------
s/getUsingNamespceDirectives/getUsingNamespaceDirectives


================
Comment at: clang-tools-extra/clangd/AST.cpp:86
+        continue;
+      if (auto *UDD = llvm::dyn_cast<UsingDirectiveDecl>(D)) {
+        VisibleNamespaceDecls.insert(
----------------
NIT: braces are redundant


================
Comment at: clang-tools-extra/clangd/AST.cpp:330
+                          [&](NestedNameSpecifier *NNS) {
+                            return llvm::any_of(
+                                VisibleNamespaceDecls,
----------------
Maybe exit early if `NNS` is not a namespace here? No need to search for it in that case.


================
Comment at: clang-tools-extra/clangd/AST.cpp:330
+                          [&](NestedNameSpecifier *NNS) {
+                            return llvm::any_of(
+                                VisibleNamespaceDecls,
----------------
ilya-biryukov wrote:
> Maybe exit early if `NNS` is not a namespace here? No need to search for it in that case.
`llvm::any_of` is `O(n)`, maybe search in `DenseSet` instead?


================
Comment at: clang-tools-extra/clangd/AST.cpp:333
+                                [NNS](const NamespaceDecl *NSD) {
+                                  return NSD == NNS->getAsNamespace();
+                                });
----------------
Could we check against `getCanonicalDecl()` here?


================
Comment at: clang-tools-extra/clangd/AST.cpp:342
+                             llvm::ArrayRef<std::string> VisibleNamespaces) {
+  return getQualification(
+      Context, DestContext, ND->getDeclContext(),
----------------
Could you assert each element in `VisibleNamespaces` ends with `::` here?


================
Comment at: clang-tools-extra/clangd/AST.cpp:345
+      [&](NestedNameSpecifier *NNS) {
+        return llvm::any_of(VisibleNamespaces, [&](llvm::StringRef Namespace) {
+          std::string NS;
----------------
That's `O(N)` in the number of `VisibleNamespaces`.
Maybe accept a `StringSet` instead?


================
Comment at: clang-tools-extra/clangd/AST.cpp:348
+          llvm::raw_string_ostream OS(NS);
+          NNS->print(OS, Context.getPrintingPolicy());
+          return OS.str() == Namespace;
----------------
This is really inefficient and this function is called on each reference inside refactored code, so can be quite costly.
But we could optimize later if this problem actually pops up in the future.


================
Comment at: clang-tools-extra/clangd/AST.h:124
+/// return bar.
+
+/// This version considers all the using namespace directives before \p
----------------
remove this newline or add a comment marker


================
Comment at: clang-tools-extra/clangd/AST.h:127
+/// InsertionPoint. i.e, if you have `using namespace
+/// clang::clangd::bar`, this function will return null, for the example above
+/// since no qualification is necessary in that case.
----------------
NIT: no comma is needed after 'return null'


================
Comment at: clang-tools-extra/clangd/AST.h:129
+/// since no qualification is necessary in that case.
+std::string getQualification(ASTContext &Context,
+                             const DeclContext *DestContext,
----------------
Current limitation of this function is it working only for namespace.
I don't think we want to dive into complexities of lookup inside classes for C++.

Should we mention that in the doc comment?


================
Comment at: clang-tools-extra/clangd/AST.h:134
+
+/// This function uses the \p VisibleNamespaces as an early exit mechanism
+/// instead of semantic using namespace directives. It can be useful if there's
----------------
Was a bit confused by the meaning of "early exit mechanism". Visible namespaces are a way to change the results of the function, yet the comment suggests it's only used to optimize and "exit early".

Could you elaborate on the intention of the comment and maybe clarify it in the code too?


================
Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:49
 
+TEST(ClangdAST, GetQualification) {
+  const struct {
----------------
Could you add a comment describing how to read test inputs and expected results?
It seems a bit non-trivial


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69608/new/

https://reviews.llvm.org/D69608