[clang-tidy] performance-for-range-copy: Don't trigger on implicit type conversions.

[flx]

Summary

This disables the check for false positive cases where implicit type conversion
through either an implicit single argument constructor or a member conversion
operator is triggered when constructing the loop variable.

Fix the test cases that meant to cover these cases.

[flx]

Remove include.

[flx]

Remove include.

[lebedev.ri]

It is best not to change existing tests, but add new ones.

[flx]

In D97577#2592093, @lebedev.ri wrote:

It is best not to change existing tests, but add new ones.

Could elaborate on this, Roman?

In this case the tests use special auto convertible types with the intention to test that the check doesn't trigger because the loop variable needs to be constructed. But the reason they don't trigger is that the loop variable erroneously is already a reference type.

If we keep the tests as is we should rename the test functions and use different types or document that the real reason they don't trigger is that they already are const reference types, but we already have a test for that as well.

[hokein]

In D97577#2592827, @flx wrote:

In D97577#2592093, @lebedev.ri wrote:

It is best not to change existing tests, but add new ones.

Could elaborate on this, Roman?

In this case the tests use special auto convertible types with the intention to test that the check doesn't trigger because the loop variable needs to be constructed. But the reason they don't trigger is that the loop variable erroneously is already a reference type.

If we keep the tests as is we should rename the test functions and use different types or document that the real reason they don't trigger is that they already are const reference types, but we already have a test for that as well.

The explanation sounds plausible, thanks.

[lebedev.ri]

In D97577#2596458, @hokein wrote:

In D97577#2592827, @flx wrote:

In D97577#2592093, @lebedev.ri wrote:

It is best not to change existing tests, but add new ones.

Could elaborate on this, Roman?

In this case the tests use special auto convertible types with the intention to test that the check doesn't trigger because the loop variable needs to be constructed. But the reason they don't trigger is that the loop variable erroneously is already a reference type.

If we keep the tests as is we should rename the test functions and use different types or document that the real reason they don't trigger is that they already are const reference types, but we already have a test for that as well.

The explanation sounds plausible, thanks.

Yep, no further comments from me.

[flx]

Thank you for the review!

clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp

 //===--- ForRangeCopyCheck.cpp - clang-tidy--------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ForRangeCopyCheck.h"
 #include "../utils/DeclRefExprUtils.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/Matchers.h"
 #include "../utils/OptionsUtils.h"
 #include "../utils/TypeTraits.h"
+#include "third_party/llvm/llvm-project/clang/include/clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "clang/Basic/Diagnostic.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace performance {
void ForRangeCopyCheck::registerMatchers(MatchFinder *Finder) {
   auto HasReferenceOrPointerTypeOrIsAllowed = hasType(qualType(
       unless(anyOf(hasCanonicalType(anyOf(referenceType(), pointerType())),
                    hasDeclaration(namedDecl(
                        matchers::matchesAnyListedName(AllowedTypes)))))));
   auto IteratorReturnsValueType = cxxOperatorCallExpr(
       hasOverloadedOperatorName("*"),
       callee(
           cxxMethodDecl(returns(unless(hasCanonicalType(referenceType()))))));
+  auto NotConstructedByCopy = cxxConstructExpr(
+      hasDeclaration(cxxConstructorDecl(unless(isCopyConstructor()))));
+  auto ConstructedByConversion = cxxMemberCallExpr(callee(cxxConversionDecl()));
   auto LoopVar =
       varDecl(HasReferenceOrPointerTypeOrIsAllowed,
-              unless(hasInitializer(expr(hasDescendant(expr(anyOf(
-                  materializeTemporaryExpr(), IteratorReturnsValueType)))))));
+              unless(hasInitializer(expr(hasDescendant(expr(
+                  anyOf(materializeTemporaryExpr(), IteratorReturnsValueType,
+                        NotConstructedByCopy, ConstructedByConversion)))))));
   Finder->addMatcher(
       traverse(TK_AsIs,
                cxxForRangeStmt(hasLoopVariable(LoopVar.bind("loopVar")))
                    .bind("forRange")),
       this);
 }
 
 void ForRangeCopyCheck::check(const MatchFinder::MatchResult &Result) {

clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy.cpp

 
 void negativeConstReference() {
   for (const S &S1 : View<Iterator<S>>()) {
   }
 }
 
 void negativeUserDefinedConversion() {
   Convertible C[0];
-  for (const S &S1 : C) {
+  for (const S S1 : C) {
   }
 }
 
 void negativeImplicitConstructorConversion() {
   ConstructorConvertible C[0];
-  for (const S &S1 : C) {
+  for (const S S1 : C) {
   }
 }
 
 template <typename T>
 void uninstantiated() {
   for (const S S1 : View<Iterator<S>>()) {}
   // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
   // CHECK-FIXES: {{^}}  for (const S& S1 : View<Iterator<S>>()) {}