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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

That's it!
Hopefully last portion of nits.



================
Comment at: clang-tidy/performance/UnnecessaryCopyInitialization.cpp:65-66
                        has(varDecl(hasLocalStorage(),
-                                   hasType(matchers::isExpensiveToCopy()),
+                                   hasType(hasCanonicalType(
+                                       allOf(matchers::isExpensiveToCopy(),
+                                             unless(hasDeclaration(namedDecl(
----------------
Does it matter whether we are calling `matchers::isExpensiveToCopy()` on the type, or on the canonical type?


================
Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:83-86
+          unless(anyOf(referenceType(),
+                       hasDeclaration(namedDecl(
+                           matchers::matchesAnyListedName(AllowedTypes))))),
+          matchers::isExpensiveToCopy()))),
----------------
This is inconsistent with the rest of the matchers here.
I think you want to check `isExpensiveToCopy()` first, because that should be less expensive than regex.


================
Comment at: clang-tidy/utils/Matchers.h:51
 
+AST_MATCHER_P(NamedDecl, matchesAnyListedName, std::vector<std::string>,
+              NameList) {
----------------
Please double-check that this does not result in a copy, and `std::vector` is passed as a reference.


================
Comment at: clang-tidy/utils/Matchers.h:53
+              NameList) {
+  for (const std::string &Name: NameList) {
+    if (llvm::Regex(Name).match(Node.getName()))
----------------
```
for (const std::string &Name : NameList) {
```


================
Comment at: clang-tidy/utils/Matchers.h:53
+              NameList) {
+  for (const std::string &Name: NameList) {
+    if (llvm::Regex(Name).match(Node.getName()))
----------------
lebedev.ri wrote:
> ```
> for (const std::string &Name : NameList) {
> ```
Do we want to be explicit about early-return here?


https://reviews.llvm.org/D52727