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

[Felix Berger via Phabricator via cfe-commits]

flx added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp:52
       AllowedTypes(
           utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
 
----------------
lebedev.ri wrote:
> flx wrote:
> > lebedev.ri wrote:
> > > Just put it here?
> > I tried this first, but this list  is substring matched against only on the non-qualified type name, so std::function would not match anything and if we added "function" here it would match many other types that contain the word function.
> I would personally say it's a bug, especially because i personally don't like hardcoded "choices" that are impossible to change.
> 
Are you referring to how the matching of "AllowedTypes" works or the fact that std::function causes false positives?

The latter is currently a bug and by excluding it now its severity is downgraded from bug to missing feature. Is your concern that you would like to have std::function be matched even if it is a false positive?

Regarding "AllowedTypes" matching I'm also surprised to find it matches substrings without namespaces as the name doesn't communicate this and makes it an imprecise net to cast. Changing it could break existing users that have type substrings configured though.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp:409
+
+namespace std {
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89332/new/

https://reviews.llvm.org/D89332