From 3e1d483e0efb64e2e6af5407bd3d1c8db2150b35 Mon Sep 17 00:00:00 2001 From: Richard Trieu Date: Mon, 13 Apr 2015 22:08:55 +0000 Subject: [PATCH] Add new warning -Wrange-loop-analysis to warn on copies during loops. -Wrange-loop-analysis is a subgroup of -Wloop-analysis and will warn when a range-based for-loop makes copies of the elements in the range. If possible, suggest the proper type to prevent copies, or the non-reference to help distinguish copy versus non-copy forms. Existing warnings in -Wloop-analysis are moved to -Wfor-loop-analysis, also a subgroup of -Wloop-analysis. Differential Revision: http://reviews.llvm.org/D4169 llvm-svn: 234804 --- clang/include/clang/Basic/DiagnosticGroups.td | 5 +- .../clang/Basic/DiagnosticSemaKinds.td | 21 +- clang/lib/Sema/SemaStmt.cpp | 152 +++++++++ .../test/SemaCXX/warn-range-loop-analysis.cpp | 299 ++++++++++++++++++ 4 files changed, 474 insertions(+), 3 deletions(-) create mode 100644 clang/test/SemaCXX/warn-range-loop-analysis.cpp diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index bb528db21d26..02d9c0cf26d8 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -222,7 +222,10 @@ def GNULabelsAsValue : DiagGroup<"gnu-label-as-value">; def LiteralRange : DiagGroup<"literal-range">; def LocalTypeTemplateArgs : DiagGroup<"local-type-template-args", [CXX98CompatLocalTypeTemplateArgs]>; -def LoopAnalysis : DiagGroup<"loop-analysis">; +def RangeLoopAnalysis : DiagGroup<"range-loop-analysis">; +def ForLoopAnalysis : DiagGroup<"for-loop-analysis">; +def LoopAnalysis : DiagGroup<"loop-analysis", [ForLoopAnalysis, + RangeLoopAnalysis]>; def MalformedWarningCheck : DiagGroup<"malformed-warning-check">; def Main : DiagGroup<"main">; def MainReturnType : DiagGroup<"main-return-type">; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 6e37856c3428..89b439c7c317 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -24,13 +24,30 @@ def note_defined_here : Note<"%0 defined here">; def warn_variables_not_in_loop_body : Warning< "variable%select{s| %1|s %1 and %2|s %1, %2, and %3|s %1, %2, %3, and %4}0 " "used in loop condition not modified in loop body">, - InGroup, DefaultIgnore; + InGroup, DefaultIgnore; def warn_redundant_loop_iteration : Warning< "variable %0 is %select{decremented|incremented}1 both in the loop header " "and in the loop body">, - InGroup, DefaultIgnore; + InGroup, DefaultIgnore; def note_loop_iteration_here : Note<"%select{decremented|incremented}0 here">; +def warn_for_range_const_reference_copy : Warning< + "loop variable %0 " + "%diff{has type $ but is initialized with type $" + "| is initialized with a value of a different type}1,2 resulting in a copy">, + InGroup, DefaultIgnore; +def note_use_type_or_non_reference : Note< + "use non-reference type %0 to keep the copy or type %1 to prevent copying">; +def warn_for_range_variable_always_copy : Warning< + "loop variable %0 is always a copy because the range of type %1 does not " + "return a reference">, + InGroup, DefaultIgnore; +def note_use_non_reference_type : Note<"use non-reference type %0">; +def warn_for_range_copy : Warning< + "loop variable %0 of type %1 creates a copy from type %2">, + InGroup, DefaultIgnore; +def note_use_reference_type : Note<"use reference type %0 to prevent copying">; + def warn_duplicate_enum_values : Warning< "element %0 has been implicitly assigned %1 which another element has " "been assigned">, InGroup>, DefaultIgnore; diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp index 8b174f8d922b..ed5da43f8a96 100644 --- a/clang/lib/Sema/SemaStmt.cpp +++ b/clang/lib/Sema/SemaStmt.cpp @@ -2364,6 +2364,156 @@ StmtResult Sema::FinishObjCForCollectionStmt(Stmt *S, Stmt *B) { return S; } +// Warn when the loop variable is a const reference that creates a copy. +// Suggest using the non-reference type for copies. If a copy can be prevented +// suggest the const reference type that would do so. +// For instance, given "for (const &Foo : Range)", suggest +// "for (const Foo : Range)" to denote a copy is made for the loop. If +// possible, also suggest "for (const &Bar : Range)" if this type prevents +// the copy altogether. +static void DiagnoseForRangeReferenceVariableCopies(Sema &SemaRef, + const VarDecl *VD, + QualType RangeInitType) { + const Expr *InitExpr = VD->getInit(); + if (!InitExpr) + return; + + QualType VariableType = VD->getType(); + + const MaterializeTemporaryExpr *MTE = + dyn_cast(InitExpr); + + // No copy made. + if (!MTE) + return; + + const Expr *E = MTE->GetTemporaryExpr()->IgnoreImpCasts(); + + // Searching for either UnaryOperator for dereference of a pointer or + // CXXOperatorCallExpr for handling iterators. + while (!isa(E) && !isa(E)) { + if (const CXXConstructExpr *CCE = dyn_cast(E)) { + E = CCE->getArg(0); + } else if (const CXXMemberCallExpr *Call = dyn_cast(E)) { + const MemberExpr *ME = cast(Call->getCallee()); + E = ME->getBase(); + } else { + const MaterializeTemporaryExpr *MTE = cast(E); + E = MTE->GetTemporaryExpr(); + } + E = E->IgnoreImpCasts(); + } + + bool ReturnsReference = false; + if (isa(E)) { + ReturnsReference = true; + } else { + const CXXOperatorCallExpr *Call = cast(E); + const FunctionDecl *FD = Call->getDirectCallee(); + QualType ReturnType = FD->getReturnType(); + ReturnsReference = ReturnType->isReferenceType(); + } + + if (ReturnsReference) { + // Loop variable creates a temporary. Suggest either to go with + // non-reference loop variable to indiciate a copy is made, or + // the correct time to bind a const reference. + SemaRef.Diag(VD->getLocation(), diag::warn_for_range_const_reference_copy) + << VD << VariableType << E->getType(); + QualType NonReferenceType = VariableType.getNonReferenceType(); + NonReferenceType.removeLocalConst(); + QualType NewReferenceType = + SemaRef.Context.getLValueReferenceType(E->getType().withConst()); + SemaRef.Diag(VD->getLocStart(), diag::note_use_type_or_non_reference) + << NonReferenceType << NewReferenceType << VD->getSourceRange(); + } else { + // The range always returns a copy, so a temporary is always created. + // Suggest removing the reference from the loop variable. + SemaRef.Diag(VD->getLocation(), diag::warn_for_range_variable_always_copy) + << VD << RangeInitType; + QualType NonReferenceType = VariableType.getNonReferenceType(); + NonReferenceType.removeLocalConst(); + SemaRef.Diag(VD->getLocStart(), diag::note_use_non_reference_type) + << NonReferenceType << VD->getSourceRange(); + } +} + +// Warns when the loop variable can be changed to a reference type to +// prevent a copy. For instance, if given "for (const Foo x : Range)" suggest +// "for (const Foo &x : Range)" if this form does not make a copy. +static void DiagnoseForRangeConstVariableCopies(Sema &SemaRef, + const VarDecl *VD) { + const Expr *InitExpr = VD->getInit(); + if (!InitExpr) + return; + + QualType VariableType = VD->getType(); + + if (const CXXConstructExpr *CE = dyn_cast(InitExpr)) { + if (!CE->getConstructor()->isCopyConstructor()) + return; + } else if (const CastExpr *CE = dyn_cast(InitExpr)) { + if (CE->getCastKind() != CK_LValueToRValue) + return; + } else { + return; + } + + // TODO: Determine a maximum size that a POD type can be before a diagnostic + // should be emitted. Also, only ignore POD types with trivial copy + // constructors. + if (VariableType.isPODType(SemaRef.Context)) + return; + + // Suggest changing from a const variable to a const reference variable + // if doing so will prevent a copy. + SemaRef.Diag(VD->getLocation(), diag::warn_for_range_copy) + << VD << VariableType << InitExpr->getType(); + SemaRef.Diag(VD->getLocStart(), diag::note_use_reference_type) + << SemaRef.Context.getLValueReferenceType(VariableType) + << VD->getSourceRange(); +} + +/// DiagnoseForRangeVariableCopies - Diagnose three cases and fixes for them. +/// 1) for (const foo &x : foos) where foos only returns a copy. Suggest +/// using "const foo x" to show that a copy is made +/// 2) for (const bar &x : foos) where bar is a temporary intialized by bar. +/// Suggest either "const bar x" to keep the copying or "const foo& x" to +/// prevent the copy. +/// 3) for (const foo x : foos) where x is constructed from a reference foo. +/// Suggest "const foo &x" to prevent the copy. +static void DiagnoseForRangeVariableCopies(Sema &SemaRef, + const CXXForRangeStmt *ForStmt) { + if (SemaRef.Diags.isIgnored(diag::warn_for_range_const_reference_copy, + ForStmt->getLocStart()) && + SemaRef.Diags.isIgnored(diag::warn_for_range_variable_always_copy, + ForStmt->getLocStart()) && + SemaRef.Diags.isIgnored(diag::warn_for_range_copy, + ForStmt->getLocStart())) { + return; + } + + const VarDecl *VD = ForStmt->getLoopVariable(); + if (!VD) + return; + + QualType VariableType = VD->getType(); + + if (VariableType->isIncompleteType()) + return; + + const Expr *InitExpr = VD->getInit(); + if (!InitExpr) + return; + + if (VariableType->isReferenceType()) { + DiagnoseForRangeReferenceVariableCopies(SemaRef, VD, + ForStmt->getRangeInit()->getType()); + } else if (VariableType.isConstQualified()) { + DiagnoseForRangeConstVariableCopies(SemaRef, VD); + } +} + /// FinishCXXForRangeStmt - Attach the body to a C++0x for-range statement. /// This is a separate step from ActOnCXXForRangeStmt because analysis of the /// body cannot be performed until after the type of the range variable is @@ -2381,6 +2531,8 @@ StmtResult Sema::FinishCXXForRangeStmt(Stmt *S, Stmt *B) { DiagnoseEmptyStmtBody(ForStmt->getRParenLoc(), B, diag::warn_empty_range_based_for_body); + DiagnoseForRangeVariableCopies(*this, ForStmt); + return S; } diff --git a/clang/test/SemaCXX/warn-range-loop-analysis.cpp b/clang/test/SemaCXX/warn-range-loop-analysis.cpp new file mode 100644 index 000000000000..91756b970e97 --- /dev/null +++ b/clang/test/SemaCXX/warn-range-loop-analysis.cpp @@ -0,0 +1,299 @@ +// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wloop-analysis -verify %s +// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wrange-loop-analysis -verify %s + +template +struct Iterator { + return_type operator*(); + Iterator operator++(); + bool operator!=(const Iterator); +}; + +template +struct Container { + typedef Iterator I; + + I begin(); + I end(); +}; + +struct Foo {}; +struct Bar { + Bar(Foo); + Bar(int); + operator int(); +}; + +// Testing notes: +// test0 checks that the full text of the warnings and notes is correct. The +// rest of the tests checks a smaller portion of the text. +// test1-6 are set in pairs, the odd numbers are the non-reference returning +// versions of the even numbers. +// test7-9 use an array instead of a range object +// tests use all four versions of the loop varaible, const &T, const T, T&, and +// T. Versions producing errors and are commented out. +// +// Conversion chart: +// double <=> int +// int <=> Bar +// double => Bar +// Foo => Bar +// +// Conversions during tests: +// test1-2 +// int => int +// int => double +// int => Bar +// test3-4 +// Bar => Bar +// Bar => int +// test5-6 +// Foo => Bar +// test7 +// double => double +// double => int +// double => Bar +// test8 +// Foo => Foo +// Foo => Bar +// test9 +// Bar => Bar +// Bar => int + +void test0() { + Container int_non_ref_container; + Container int_container; + Container bar_container; + + for (const int &x : int_non_ref_container) {} + // expected-warning@-1 {{loop variable 'x' is always a copy because the range of type 'Container' does not return a reference}} + // expected-note@-2 {{use non-reference type 'int'}} + + for (const double &x : int_container) {} + // expected-warning@-1 {{loop variable 'x' has type 'const double &' but is initialized with type 'int' resulting in a copy}} + // expected-note@-2 {{use non-reference type 'double' to keep the copy or type 'const int &' to prevent copying}} + + for (const Bar x : bar_container) {} + // expected-warning@-1 {{loop variable 'x' of type 'const Bar' creates a copy from type 'const Bar'}} + // expected-note@-2 {{use reference type 'const Bar &' to prevent copying}} +} + +void test1() { + Container A; + + for (const int &x : A) {} + // expected-warning@-1 {{always a copy}} + // expected-note@-2 {{'int'}} + for (const int x : A) {} + // No warning, non-reference type indicates copy is made + //for (int &x : A) {} + // Binding error + for (int x : A) {} + // No warning, non-reference type indicates copy is made + + for (const double &x : A) {} + // expected-warning@-1 {{always a copy}} + // expected-note@-2 {{'double'}} + for (const double x : A) {} + // No warning, non-reference type indicates copy is made + //for (double &x : A) {} + // Binding error + for (double x : A) {} + // No warning, non-reference type indicates copy is made + + for (const Bar &x : A) {} + // expected-warning@-1 {{always a copy}} + // expected-note@-2 {{'Bar'}} + for (const Bar x : A) {} + // No warning, non-reference type indicates copy is made + //for (Bar &x : A) {} + // Binding error + for (Bar x : A) {} + // No warning, non-reference type indicates copy is made +} + +void test2() { + Container B; + + for (const int &x : B) {} + // No warning, this reference is not a temporary + for (const int x : B) {} + // No warning on POD copy + for (int &x : B) {} + // No warning + for (int x : B) {} + // No warning + + for (const double &x : B) {} + // expected-warning@-1 {{resulting in a copy}} + // expected-note-re@-2 {{'double'{{.*}}'const int &'}} + for (const double x : B) {} + //for (double &x : B) {} + // Binding error + for (double x : B) {} + // No warning + + for (const Bar &x : B) {} + // expected-warning@-1 {{resulting in a copy}} + // expected-note@-2 {{'Bar'}} + for (const Bar x : B) {} + //for (Bar &x : B) {} + // Binding error + for (Bar x : B) {} + // No warning +} + +void test3() { + Container C; + + for (const Bar &x : C) {} + // expected-warning@-1 {{always a copy}} + // expected-note@-2 {{'Bar'}} + for (const Bar x : C) {} + // No warning, non-reference type indicates copy is made + //for (Bar &x : C) {} + // Binding error + for (Bar x : C) {} + // No warning, non-reference type indicates copy is made + + for (const int &x : C) {} + // expected-warning@-1 {{always a copy}} + // expected-note@-2 {{'int'}} + for (const int x : C) {} + // No warning, copy made + //for (int &x : C) {} + // Binding error + for (int x : C) {} + // No warning, copy made +} + +void test4() { + Container D; + + for (const Bar &x : D) {} + // No warning, this reference is not a temporary + for (const Bar x : D) {} + // expected-warning@-1 {{creates a copy}} + // expected-note@-2 {{'const Bar &'}} + for (Bar &x : D) {} + // No warning + for (Bar x : D) {} + // No warning + + for (const int &x : D) {} + // expected-warning@-1 {{resulting in a copy}} + // expected-note-re@-2 {{'int'{{.*}}'const Bar &'}} + for (const int x : D) {} + // No warning + //for (int &x : D) {} + // Binding error + for (int x : D) {} + // No warning +} + +void test5() { + Container E; + + for (const Bar &x : E) {} + // expected-warning@-1 {{always a copy}} + // expected-note@-2 {{'Bar'}} + for (const Bar x : E) {} + // No warning, non-reference type indicates copy is made + //for (Bar &x : E) {} + // Binding error + for (Bar x : E) {} + // No warning, non-reference type indicates copy is made +} + +void test6() { + Container F; + + for (const Bar &x : F) {} + // expected-warning@-1 {{resulting in a copy}} + // expected-note-re@-2 {{'Bar'{{.*}}'const Foo &'}} + for (const Bar x : F) {} + // No warning. + //for (Bar &x : F) {} + // Binding error + for (Bar x : F) {} + // No warning +} + +void test7() { + double G[2]; + + for (const double &x : G) {} + // No warning + for (const double x : G) {} + // No warning on POD copy + for (double &x : G) {} + // No warning + for (double x : G) {} + // No warning + + for (const int &x : G) {} + // expected-warning@-1 {{resulting in a copy}} + // expected-note-re@-2 {{'int'{{.*}}'const double &'}} + for (const int x : G) {} + // No warning + //for (int &x : G) {} + // Binding error + for (int x : G) {} + // No warning + + for (const Bar &x : G) {} + // expected-warning@-1 {{resulting in a copy}} + // expected-note-re@-2 {{'Bar'{{.*}}'const double &'}} + for (const Bar x : G) {} + // No warning + //for (int &Bar : G) {} + // Binding error + for (int Bar : G) {} + // No warning +} + +void test8() { + Foo H[2]; + + for (const Foo &x : H) {} + // No warning + for (const Foo x : H) {} + // No warning on POD copy + for (Foo &x : H) {} + // No warning + for (Foo x : H) {} + // No warning + + for (const Bar &x : H) {} + // expected-warning@-1 {{resulting in a copy}} + // expected-note-re@-2 {{'Bar'{{.*}}'const Foo &'}} + for (const Bar x : H) {} + // No warning + //for (Bar &x: H) {} + // Binding error + for (Bar x: H) {} + // No warning +} + +void test9() { + Bar I[2] = {1,2}; + + for (const Bar &x : I) {} + // No warning + for (const Bar x : I) {} + // expected-warning@-1 {{creates a copy}} + // expected-note@-2 {{'const Bar &'}} + for (Bar &x : I) {} + // No warning + for (Bar x : I) {} + // No warning + + for (const int &x : I) {} + // expected-warning@-1 {{resulting in a copy}} + // expected-note-re@-2 {{'int'{{.*}}'const Bar &'}} + for (const int x : I) {} + // No warning + //for (int &x : I) {} + // Binding error + for (int x : I) {} + // No warning +}