[PATCH] D52727: [clang-tidy] White List Option for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:49
+  // Skip whitelisted types
+  const auto VarType = Var->getType();
+  if (std::find_if(WhiteListTypes.begin(), WhiteListTypes.end(),
----------------
lebedev.ri wrote:
> I'm not sure what is `auto` here, please spell `QualType`.
And please elide `const`, as it is a value and values are not made `const` in llvm code (consistency for now)


================
Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:50
+  const auto VarType = Var->getType();
+  if (std::find_if(WhiteListTypes.begin(), WhiteListTypes.end(),
+                   [&](llvm::StringRef WhiteListType) {
----------------
lebedev.ri wrote:
> `llvm::any_of()`
This configuration should be used in the matchers. Please see `cppcoreguidelines-no-malloc` for an example on how to do it in the matchers. Having it there usually improves performance and is clearer.


================
Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:110
+
+  const bool IsConstQualified = ParamType.getCanonicalType().isConstQualified();
 
----------------
please remove the `const`


================
Comment at: docs/clang-tidy/checks/performance-for-range-copy.rst:31
+
+   A semicolon-separated list of names of whitelist types. Regular expressions
+   are allowed. Default is empty.
----------------
Please give an example for regular expressions. There are many slightly different variations of them and not everyone might be familiar. I think one wildcard expression is enough.

The sentences miss some fill words i think. `whitelisted types`, `The default is empty`.


================
Comment at: docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst:44
+
+   A semicolon-separated list of names of whitelist types. Regular expressions
+   are allowed. Default is empty.
----------------
Same as other doc, same below


================
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.WhiteListTypes, value: '[Pp]ointer$|[Pp]tr$|[Rr]ef(erence)?$'}]}" -- -std=c++11 -fno-delayed-template-parsing
 
----------------
I would prefer 2 test files. One with default configuration and one with the special whitelisting, same for the other checks


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52727