This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Remove using-namespace present inside a namespace.
Needs ReviewPublic

Authored by usaxena95 on Oct 18 2019, 3:41 AM.

Details

Reviewers
ilya-biryukov
Summary

This patch extends the removing using-namespace code action to remove
using-namespace decl that are present inside a namespace.

Goes over all the references that are declared in TargetNS and used in
ContainingNS. Removes the current qualifier and qualifies it with only
the TargetNS. No other qualifier is needed since it was legal to use
using namespace TargetNS inside ContainingNS. Therefore all target
references inside the ContainingNS can be qualified with just
TargetNS.

Limitations:

  • The using decl must be present in TUDecl or directly under a NSDecl.
  • Only references inside the DeclContext of using-decl are removed. Any references outside this DeclContext are retained as is which is incorrect.

Diff Detail

Event Timeline

usaxena95 created this revision.Oct 18 2019, 3:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2019, 3:41 AM
usaxena95 edited the summary of this revision. (Show Details)Oct 18 2019, 3:47 AM

Build result: fail - 33547 tests passed, 1 failed and 464 were skipped.

failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

usaxena95 updated this revision to Diff 225596.Oct 18 2019, 4:10 AM

Added additional tests.

Build result: fail - 33547 tests passed, 1 failed and 464 were skipped.

failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

The most important comment is in the tests. Is there a way to have the same effect with less changes?

clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
47

NIT: could you add a short comment mentioning this is exactly the DeclContext of TargetDirective?

110

This check looks redundant now that we check the decl context is a namespace of a translation unit.
I think it was originally intended to check the whether the using namespace is inside a function or not.

173

NIT: maybe simplify to

QualifierReplacements.push_back(
  CharSourceRange::getTokenRange(
    Ref.Qualifier ? Ref.Qualifier.getBeginLoc : Loc, 
    Loc));

?

180

Do we also need to run in the redeclarations of the same namespace?

namespace clangd {
  using namespace clang;
}

namespace clangd {
  /*clang::*/Decl* D = nullptr;
}
182

NIT: could you add braces around the branches of the if statement?
for loop is complicated, so it probably qualifies as something that asks for the braces.

clang-tools-extra/clangd/unittests/TweakTests.cpp
898

Is there a way to avoid re-qualifying this?
We should aim for minimal changes and there is probably a simple way to achieve this by only qualifying when the qualifier itself references the ContainingNS and target is inside the target namespace of the directive being removed.

Or this too complicated for some reason?

usaxena95 updated this revision to Diff 226962.Oct 29 2019, 1:49 PM
usaxena95 marked 7 inline comments as done.

Addressed comments.

usaxena95 added inline comments.Oct 29 2019, 1:51 PM
clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
180

Aah. Didn't know we have the redecls here itself. I had a FIXME as a negative test for this. Fixed that now.

Build result: fail - 59446 tests passed, 1 failed and 764 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_recursive/lock.pass.cpp

Log files: console-log.txt, CMakeCache.txt

One important question about running on the whole TU in all cases. Other than that LG

clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
111

NIT: use auto*
knowing it's a pointer is quite useful when reading and writing the code (e.g. suggests one need to use -> instead of . to access the variable)

179

Could you explain why don't we always just run across the whole TU?
What disadvantages would that have?

182

NIT: remove empty line

usaxena95 marked 3 inline comments as done.Oct 31 2019, 4:47 AM
usaxena95 added inline comments.
clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
179

Working with a using decl inside the ContainingNS is quite similar to our initial version with GlobalNS.
For example we take advantage of that to replace the whole qualifier with just TargetNS. We use the fact that since using namespace TargetNS; was valid inside ContainingNS then qualifying the reference with just TargetNS would also be valid inside ContainingNS.

When we are outside ContainingNS, we would have to fully qualify the ref to be 100% correct. To reduce the qualification we might have to figure out which is enclosing NS for the reference.

namespace aa {
namespace bb { 
struct map {}; 
}
using namespace b^b;
}
int main() { 
 aa::map m1;  // We do not qualify this currently.
}
usaxena95 updated this revision to Diff 227267.Oct 31 2019, 4:48 AM

Addressed comments.

Build result: fail - 33521 tests passed, 1 failed and 463 were skipped.

failed: LLVM.tools/llvm-objdump/X86/disassemble-functions.test

Log files: console-log.txt, CMakeCache.txt

ilya-biryukov added inline comments.Nov 4 2019, 8:49 AM
clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
111

NIT: you could use DC here, clangd uses Go-style short identifiers for locals and parameters extensively

146

NIT: Maybe call it QualifyRef? Was a bit confused by Select

179

If we detect references outside ContainingNS, which are definitely broken, fully-qualifying seems better than silently ignoring those.

I think they're quite easy to detect:

  • Their target should be inside TargetNS,
  • Their QualifierLoc should reference ContainingNS.

We know that those won't work anymore, so we should qualify them.

Leaving broken code is definitely a bad idea, if we can easily detect and fix it.