[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:39
+  auto WhitelistClassMatcher =
+      cxxRecordDecl(hasAnyName(SmallVector<StringRef, 4>(
+          WhitelistClasses.begin(), WhitelistClasses.end())));
----------------
JonasToth wrote:
> I have seens this pattern now multiple times in various checks, we should have some utility wrapping the `hasAnyName(MyList)`. (Just in general, does not need to happen with this check)
no needed for this patch. But yes! Moving to utility is reasonable to me.


================
Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:93
     return false;
   if (utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
           .isMutated(&LoopVar))
----------------
JonasToth wrote:
> Adding a `..IsMutated(&LoopVar) && IsReferenced(ForScope, LoopVar))` here should fix the warning as well.
I think ignoring `for (auto _ : state)` is a sensible solution. Thanks!


================
Comment at: test/clang-tidy/performance-for-range-copy.cpp:1
-// RUN: %check_clang_tidy %s performance-for-range-copy %t -- -- -std=c++11 -fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s performance-for-range-copy %t -config="{CheckOptions: [{key: "performance-for-range-copy.WhitelistClasses", value: "WhitelistFoo"}]}" -- -std=c++11 -fno-delayed-template-parsing
 
----------------
JonasToth wrote:
> I would prefer a single file, that has the configuration and leave the standard test like it is.
> 
> with this, you can always track what is actually done by default and what is done with different conigurations + potential inconsistencies that might occur by bugs in the configured code.
no needed this configuration now.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50447