hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2673
+void foo::fun() {
+ one::two::f^f();
+})cpp",
----------------
adamcz wrote:
> hokein wrote:
> > IIUC, before this patch, the using was added inside the above namespace
> > `foo`, the code was still valid after applying the tweak.
> >
> > There is no enclosing namespace for `foo::fun()`, so we will insert the
> > using at the first top-level decl. It seems that we will break the code if
> > the `one::two::ff` decl is defined in the main file (not the `test.hpp`),
> > e.g.
> >
> > ```
> > // using one::two::ff will be inserted here, which is not valid
> > namespace one {
> > namespace two {
> > void ff();
> > }
> > }
> >
> > namespace foo { void fun(); }
> > void foo::fun() {...}
> > ```
> >
> > this case was working before the patch, and now is broken after this patch.
> > It is not a common case, probably ok for now.
> You are correct that this will not work. It is not a regression, however,
> since it was broken before too. Prior to this patch, the using would be
> inserted inside the namespace foo { }, so while the using statement would
> compile, it wouldn't affect the out-of-line definition of the function,
> meaning removing the qualifier would not work.
>
> The insertion point logic will have to change a little when we add the
> feature to remove all other full qualifiers for that name. I think it might
> be best to address these corner cases at that point, but I can also do it
> here if you prefer.
> You are correct that this will not work. It is not a regression, however,
> since it was broken before too. Prior to this patch, the using would be
> inserted inside the namespace foo { }, so while the using statement would
> compile, it wouldn't affect the out-of-line definition of the function,
> meaning removing the qualifier would not work.
hmm, that's interesting, the following code is compilable.
```
namespace one {
namespace two {
void ff();
}
}
namespace foo {
using one::two::ff;
void fun();
} // namespace foo
void foo::fun() { ff(); }
```
> The insertion point logic will have to change a little when we add the
> feature to remove all other full qualifiers for that name. I think it might
> be best to address these corner cases at that point, but I can also do it
> here if you prefer.
agree to defer it, this is something we should keep in mind.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79496/new/
https://reviews.llvm.org/D79496
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
