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