Adds fixit hints to the Wrange-loop-analysis

[aaron.ballman]

clang/lib/Sema/SemaStmt.cpp

2765–2766

I don't think the diagnostic should point to the end of the type specification rather than the beginning. That puts the caret in a rather strange location for the diagnostic.

[Mordante]

Undo the changes to the caret position as requested by @aaron.ballman .

[aaron.ballman]

LGTM!

[thakis]

Mordante, do you need someone to land this for you?

[Mordante]

In D68913#1787784, @thakis wrote:

Mordante, do you need someone to land this for you?

Thanks for the offer, but it's not required. I want to commit it once I fixed all issues that would be introduced by D68912

clang/include/clang/AST/Decl.h

TemplateParameterList *getTemplateParameterList(unsigned index) const {
     assert(index < getNumTemplateParameterLists());
     return getExtInfo()->TemplParamLists[index];
   }
 
   void setTemplateParameterListsInfo(ASTContext &Context,
                                      ArrayRef<TemplateParameterList *> TPLists);
 
   SourceLocation getTypeSpecStartLoc() const;
+  SourceLocation getTypeSpecEndLoc() const;
 
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) {
     return K >= firstDeclarator && K <= lastDeclarator;
   }
 };
 

clang/lib/AST/Decl.cpp

 }
 
 SourceLocation DeclaratorDecl::getTypeSpecStartLoc() const {
   TypeSourceInfo *TSI = getTypeSourceInfo();
   if (TSI) return TSI->getTypeLoc().getBeginLoc();
   return SourceLocation();
 }
 
+SourceLocation DeclaratorDecl::getTypeSpecEndLoc() const {
+  TypeSourceInfo *TSI = getTypeSourceInfo();
+  if (TSI) return TSI->getTypeLoc().getEndLoc();
+  return SourceLocation();
+}
+
 void DeclaratorDecl::setQualifierInfo(NestedNameSpecifierLoc QualifierLoc) {
   if (QualifierLoc) {
     // Make sure the extended decl info is allocated.
     if (!hasExtInfo()) {
       // Save (non-extended) type source info pointer.
       auto *savedTInfo = DeclInfo.get<TypeSourceInfo*>();
       // Allocate external info struct.
       DeclInfo = new (getASTContext()) ExtInfo;

clang/lib/Sema/SemaStmt.cpp

if (ReturnsReference) {
     // 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->getBeginLoc(), diag::note_use_type_or_non_reference)
-        << NonReferenceType << NewReferenceType << VD->getSourceRange();
+        << NonReferenceType << NewReferenceType << VD->getSourceRange()
+        << FixItHint::CreateRemoval(VD->getTypeSpecEndLoc());
   } 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->getBeginLoc(), diag::note_use_non_reference_type)
-        << NonReferenceType << VD->getSourceRange();
+        << NonReferenceType << VD->getSourceRange()
+        << FixItHint::CreateRemoval(VD->getTypeSpecEndLoc());
   }
 }
 
 // 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) {
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->getBeginLoc(), diag::note_use_reference_type)
       << SemaRef.Context.getLValueReferenceType(VariableType)
-      << VD->getSourceRange();
+      << VD->getSourceRange()
+      << FixItHint::CreateInsertion(VD->getLocation(), "&");
 }
 
 /// 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 initialized by bar.
 ///    Suggest either "const bar x" to keep the copying or "const foo& x" to
 ///    prevent the copy.

clang/test/SemaCXX/warn-range-loop-analysis.cpp

 // 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
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wloop-analysis -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 template <typename return_type>
 struct Iterator {
   return_type operator*();
   Iterator operator++();
   bool operator!=(const Iterator);
 };
 

 void test0() {
   Container<int> int_non_ref_container;
   Container<int&> int_container;
   Container<Bar&> 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<int>' does not return a reference}}
   // expected-note@-2 {{use non-reference type 'int'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
 
   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}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:21-[[@LINE-3]]:22}:""
 
   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}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:18}:"&"
 }
 
 void test1() {
   Container<int> A;
 
   for (const int &x : A) {}
   // expected-warning@-1 {{always a copy}}
   // expected-note@-2 {{'int'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
   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'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:21-[[@LINE-3]]:22}:""
   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'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
   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<int&> 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 &'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:21-[[@LINE-3]]:22}:""
   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'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
   for (const Bar x : B) {}
   //for (Bar &x : B) {}
   // Binding error
   for (Bar x : B) {}
   // No warning
 }
 
 void test3() {
   Container<Bar> C;
 
   for (const Bar &x : C) {}
   // expected-warning@-1 {{always a copy}}
   // expected-note@-2 {{'Bar'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
   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'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
   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<Bar&> 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 &'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:18}:"&"
   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 &'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
   for (const int x : D) {}
   // No warning
   //for (int &x : D) {}
   // Binding error
   for (int x : D) {}
   // No warning
 }
 
 void test5() {
   Container<Foo> E;
 
   for (const Bar &x : E) {}
   // expected-warning@-1 {{always a copy}}
   // expected-note@-2 {{'Bar'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
   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<Foo&> F;
 
   for (const Bar &x : F) {}
   // expected-warning@-1 {{resulting in a copy}}
   // expected-note-re@-2 {{'Bar'{{.*}}'const Foo &'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
   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 &'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
   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 &'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
   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 &'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
   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 &'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:18}:"&"
   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 &'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
   for (const int x : I) {}
   // No warning
   //for (int &x : I) {}
   // Binding error
   for (int x : I) {}
   // No warning
 }
+
+void test10() {
+  Container<Bar> C;
+
+  for (const Bar &x : C) {}
+  // expected-warning@-1 {{always a copy}}
+  // expected-note@-2 {{'Bar'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:19}:""
+
+  for (const Bar& x : C) {}
+  // expected-warning@-1 {{always a copy}}
+  // expected-note@-2 {{'Bar'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:""
+
+  for (const Bar & x : C) {}
+  // expected-warning@-1 {{always a copy}}
+  // expected-note@-2 {{'Bar'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:18-[[@LINE-3]]:20}:""
+
+  for (const Bar&x : C) {}
+  // expected-warning@-1 {{always a copy}}
+  // expected-note@-2 {{'Bar'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:" "
+}