flx marked 2 inline comments as done. flx added a comment. Thank you all for the input!
In D89332#2336566 <https://reviews.llvm.org/D89332#2336566>, @njames93 wrote: > How does this type alias and typedef, In theory that should also be handled. > > using Functor = std::function<...>; > Functor Copy = Orig; // No warning. Good point. I added test cases that cover this and motivated the use of the `hasName()` matcher on the canonical type. In D89332#2338319 <https://reviews.llvm.org/D89332#2338319>, @njames93 wrote: > Come to think of it, this is a pretty illogical way to solve this problem, > just append `::std::function` to the AllowedTypes vector in > `registerMatchers` and be do with it. Will require dropping the const > Qualifier on AllowedTypes, but aside from that it is a much simpler fix. > The has(Any)Name matcher has logic for skipping inline namespaces. `AllowedTypes` is currently matched using a regular expression on only the not fully qualified name of the NamedDecl: https://github.com/llvm/llvm-project/blob/343410d1cc154db99b7858e0a9c3ffd86ad94e45/clang-tools-extra/clang-tidy/utils/Matchers.h#L49 This is why this approach didn't work. ================ Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp:39-44 +AST_MATCHER(NamedDecl, isStdFunction) { + // First use the fast getName() method to avoid unnecessary calls to the + // slow getQualifiedNameAsString(). + return Node.getName() == "function" && + Node.getQualifiedNameAsString() == "std::function"; +} ---------------- aaron.ballman wrote: > njames93 wrote: > > It's better to use `node.isInStdNamespace()` instead of checking the > > qualified name as its designed for that very purpose. Is should probably > > take a `CXXRecordDecl` instead of a `NamedDecl` aswell. > There's no need for this matcher -- `hasName("::std::function")` is the > correct way to test for this. hasName() on the canonical type worked and handled type aliases as well. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp:409 + +namespace std { + ---------------- njames93 wrote: > gribozavr2 wrote: > > flx wrote: > > > gribozavr2 wrote: > > > > Could you add a nested inline namespace to better imitate what > > > > declarations look like in libc++? > > > I'm not sure I follow. I looked through the other tests that declare a > > > std function and copied the declaration from modernize-avoid-bind.cpp. > > libc++ declarations look like this: > > > > ``` > > namespace std { > > inline namespace __1 { > > template<...> struct function... > > } // __1 > > } // std > > ``` > > > > The inline namespace in the middle often trips up declaration matching in > > checkers. And yes, many other tests don't imitate this pattern, and are > > often broken with libc++. Those tests should be improved. > @flx Thats the reason why its advisable to use `Decl::isInStdNamespace` as it > will handle inline namespaces. Thanks for the extra explanation and sample code. This is done now and works. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89332/new/ https://reviews.llvm.org/D89332 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
