[PATCH] D69033: [clangd] Improve symbol qualification in DefineInline code action

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

We seem to have trouble only in presence of using declarations and using namespace directives.
I wonder how complicated it would be to take them into account instead. That would clearly be easier to read, as we'll hit right into the center of the problem.

Could you describe why handling using declarations and using namespace directives looks too complicated?



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:149
+// CurContext and SourceContext, doesn't take using declarations and directives
+// into account.
+NestedNameSpecifier *getQualification(ASTContext &Context,
----------------
Could you add an example to the description?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:152
+                                      const DeclContext *DestContext,
+                                      const DeclContext *SourceContext) {
+  // Goes over all parents of SourceContext until we find a comman ancestor
----------------
Maybe accept a NamedDecl we need to qualify instead of `SourceContext`?
This would help with avoiding confusion with two parameters of the same type and I believe we'll need it if we ever want to support using declarations in the same function.




================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:168
+    auto *NSD = llvm::dyn_cast<NamespaceDecl>(Context);
+    assert(NSD && "Non-namespace decl context found.");
+    // Again, ananoymous namespaces are not spelled while qualifying a name.
----------------
It might be possible to have non-namespace contexts here:
```
namespace ns {
struct X {
  void foo();

  static void target_struct();
};
void target_ns();
}


void ns::X::foo() {
  // we start with non-namespace context.
  target_struct();
  target_ns(); 
}
```

Not sure if we run in those cases, though


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1417
+
+  using namespace a;
+  using namespace a::b;
----------------
We don't support `using namespace` and yet we choose to use them in the tests here.
Is there a case where we need to qualify without using namespace directive and using declarations?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69033