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