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