This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Helper for getting nested namespace qualification
ClosedPublic

Authored by kadircet on Oct 30 2019, 1:11 AM.

Details

Summary

Introduce a new helper for getting minimally required qualifiers
necessary to spell a name at a point in a given DeclContext. Currently takes
using directives and nested namespecifier of DeclContext itself into account.

Initially will be used in define inline and outline actions.

Diff Detail

Event Timeline

kadircet created this revision.Oct 30 2019, 1:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2019, 1:11 AM
ilya-biryukov added inline comments.Nov 5 2019, 4:38 AM
clang-tools-extra/clangd/AST.cpp
355

NIT: dyn_cast + assert are equivalent to a single llvm::cast

356

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?

361

NIT: maybe check this alongside the Context->isInlineNamespace()?

auto *NSD = ...;
if (NSD->isAnonymousNamespace() || NSD->isInlineNamespace())
  continue;
clang-tools-extra/clangd/AST.h
121

NIT: using namespace directives

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

kadircet updated this revision to Diff 230211.Nov 20 2019, 1:37 AM
kadircet marked 4 inline comments as done.
  • Add support for non-namespace decl contexts.
  • Add an overload for "textual namespace" handling.

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

clang-tools-extra/clangd/AST.cpp
79

s/getUsingNamespceDirectives/getUsingNamespaceDirectives

88

NIT: braces are redundant

329

Maybe exit early if NNS is not a namespace here? No need to search for it in that case.

329

llvm::any_of is O(n), maybe search in DenseSet instead?

332

Could we check against getCanonicalDecl() here?

341

Could you assert each element in VisibleNamespaces ends with :: here?

344

That's O(N) in the number of VisibleNamespaces.
Maybe accept a StringSet instead?

347

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.

clang-tools-extra/clangd/AST.h
124

remove this newline or add a comment marker

127

NIT: no comma is needed after 'return null'

129

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?

134

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?

clang-tools-extra/clangd/unittests/ASTTests.cpp
51

Could you add a comment describing how to read test inputs and expected results?
It seems a bit non-trivial

ilya-biryukov accepted this revision.Nov 21 2019, 3:26 AM

LGTM when the comments are addressed

This revision is now accepted and ready to land.Nov 21 2019, 3:26 AM
kadircet updated this revision to Diff 230423.Nov 21 2019, 4:33 AM
kadircet marked 14 inline comments as done.
  • Address comments
kadircet marked 14 inline comments as done.Nov 21 2019, 4:33 AM
kadircet added inline comments.
clang-tools-extra/clangd/AST.cpp
329

i didn't bother optimizing since it is really rare to have more than 10 using namespace directives for any given file.

Build result: pass - 60224 tests passed, 0 failed and 732 were skipped.
Log files: console-log.txt, CMakeCache.txt

This revision was automatically updated to reflect the committed changes.
kadircet marked an inline comment as done.