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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 5 inline comments as done.
kadircet added a comment.

In D69033#1710955 <https://reviews.llvm.org/D69033#1710955>, @ilya-biryukov wrote:

> 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?


As discussed offline, changed the patch to handle using directives. Using declarations are handled implicitly, as we bail out if they are not visible from target
location.



================
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.
----------------
ilya-biryukov wrote:
> 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
the SourceContext is guaranteed to be a namespace context to be start with, since we only call this function in `qualifyAllDecls` after making sure current decl is a namespace decl.
So there is no way for any of its parents to be anything but a namespacedecl.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1417
+
+  using namespace a;
+  using namespace a::b;
----------------
ilya-biryukov wrote:
> 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?
> We don't support `using namespace` and yet we choose to use them in the tests here.

I believe you misunderstood the "doesn't take using directives into account" part. It is not that we don't support them, it is just the `getQualification` function generates suboptimal specifiers in the presence of using directives/declarations. For example:

```
namespace ns1{
 namespace ns2 { void foo(); }
 using namespace ns2;
 void bar();

 void bar() {
     foo();
 }
}
```

when we issue an inline on function `bar` the body will become `ns2::foo` instead of just `foo` because we didn't take `using namespace ns2` into account.



> Is there a case where we need to qualify without using namespace directive and using declarations?

if there are no using directive/declarations then the visible scope of declaration and definition should hopefully be the same and we wouldn't need to qualify anything.
But as I mentioned, it is not that we are not supporting those, we are just not producing 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69033