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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/AST.cpp:281
+
+    auto *NSD = llvm::dyn_cast<NamespaceDecl>(Context);
+    assert(NSD && "Non-namespace decl context found.");
----------------
NIT: `dyn_cast` + `assert` are equivalent to a single `llvm::cast`


================
Comment at: clang-tools-extra/clangd/AST.cpp:282
+    auto *NSD = llvm::dyn_cast<NamespaceDecl>(Context);
+    assert(NSD && "Non-namespace decl context found.");
+    // Stop if this namespace is already visible at DestContext.
----------------
This can actually fail with non-namespace decl contexts, right?
Now that we expose this as a public helper, that's actually an important failure mode that we have to care about.

Maybe change the interface to accept `NamespaceDecl` or add assertions at the start of the function?


================
Comment at: clang-tools-extra/clangd/AST.cpp:287
+    // Again, ananoymous namespaces are not spelled while qualifying a name.
+    if (NSD->isAnonymousNamespace())
+      continue;
----------------
NIT: maybe check this alongside the `Context->isInlineNamespace()`?

```
auto *NSD = ...;
if (NSD->isAnonymousNamespace() || NSD->isInlineNamespace())
  continue;
```



================
Comment at: clang-tools-extra/clangd/AST.h:118
+/// such that it is visible in \p DestContext, considering all the using
+/// directives before \p InsertionPoint.
+/// Returns null if no qualification is necessary. For example, if you want to
----------------
NIT: using **namespace** directives

otherwise it's too easy to confuse those with using declarations, i.e. `using ns::foo;`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69608