adamcz marked an inline comment as done.
adamcz added inline comments.

================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2673
+void foo::fun() {
+  one::two::f^f();
+})cpp",
----------------
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.


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

Reply via email to