[PATCH] D89332: [clang-tidy] performance-unnecessary-copy-initialization: Always allow std::function to be copied.

[Felix Berger via Phabricator via cfe-commits]

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