[clang] Add fixit for Wreorder-ctor

[njames93]

Summary

Create fix-it hints to fix the order of constructors.
To make this a lot simpler, I've grouped all the warnings for each out of order initializer into 1.
This is necessary as fixing one initializer would often interfere with other initializers.

[njames93]

Fix formatting issues

[njames93]

Arc got confused

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8611

Hmmm, how about initializer order does not match the declaration order or something along those lines? The "some" and "in the correct order" feel a bit hand-wavy for a warning.

clang/lib/Sema/SemaDeclCXX.cpp

5242

5247

5304

[njames93]

clang/lib/Sema/SemaDeclCXX.cpp

5242

This was copied from the old impl, Not sure it this is going to change behaviour though.
No tests needed to be updated though,

[njames93]

Address nits.

[aaron.ballman]

Thanks for this! The changes LGTM, but wait to land it for a day or two in case @rsmith has concerns.

clang/lib/Sema/SemaDeclCXX.cpp

5242

The diagnostic engine knows how to format anything that's a NamedDecl (including quoting it properly, etc), but it does the same work for DeclarationName. There shouldn't be a change in behavior in this case, just a slight simplification of code.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_mem_init_not_member_or_class : Error<
   "member initializer %0 does not name a non-static data member or base "
   "class">;
 
 def warn_initializer_out_of_order : Warning<
   "%select{field|base class}0 %1 will be initialized after "
   "%select{field|base}2 %3">,
   InGroup<ReorderCtor>, DefaultIgnore;
+
+def warn_some_initializers_out_of_order : Warning<
+  "initializer order does not match the declaration order">,
+  InGroup<ReorderCtor>, DefaultIgnore;
+
+def note_initializer_out_of_order : Note<
+  "%select{field|base class}0 %1 will be initialized after "
+  "%select{field|base}2 %3">;
+
 def warn_abstract_vbase_init_ignored : Warning<
   "initializer for virtual base class %0 of abstract class %1 "
   "will never be used">,
   InGroup<DiagGroup<"abstract-vbase-init">>, DefaultIgnore;
 
 def err_base_init_does_not_name_class : Error<
   "constructor initializer %0 does not name a class">;
 def err_base_init_direct_and_virtual : Error<

clang/lib/Sema/SemaDeclCXX.cpp

 static const void *GetKeyForMember(ASTContext &Context,
                                    CXXCtorInitializer *Member) {
   if (!Member->isAnyMemberInitializer())
     return GetKeyForBase(Context, QualType(Member->getBaseClass(), 0));
 
   return Member->getAnyMember()->getCanonicalDecl();
 }
 
+static void AddInitializerToDiag(const Sema::SemaDiagnosticBuilder &Diag,
+                                 const CXXCtorInitializer *Previous,
+                                 const CXXCtorInitializer *Current) {
+  if (Previous->isAnyMemberInitializer())
+    Diag << 0 << Previous->getAnyMember();
+  else
+    Diag << 1 << Previous->getTypeSourceInfo()->getType();
+
+  if (Current->isAnyMemberInitializer())
+    Diag << 0 << Current->getAnyMember();
+  else
+    Diag << 1 << Current->getTypeSourceInfo()->getType();
+}
+
 static void DiagnoseBaseOrMemInitializerOrder(
     Sema &SemaRef, const CXXConstructorDecl *Constructor,
     ArrayRef<CXXCtorInitializer *> Inits) {
   if (Constructor->getDeclContext()->isDependentContext())
     return;
 
   // Don't check initializers order unless the warning is enabled at the
   // location of at least one initializer.
if (Field->isUnnamedBitfield())
       continue;
 
     PopulateKeysForFields(Field, IdealInitKeys);
   }
 
   unsigned NumIdealInits = IdealInitKeys.size();
   unsigned IdealIndex = 0;
 
-  CXXCtorInitializer *PrevInit = nullptr;
+  // Track initializers that are in an incorrect order for either a warning or
+  // note if multiple ones occur.
+  SmallVector<unsigned> WarnIndexes;
+  // Correlates the index of an initializer in the init-list to the index of
+  // the field/base in the class.
+  SmallVector<std::pair<unsigned, unsigned>, 32> CorrelatedInitOrder;
+
   for (unsigned InitIndex = 0; InitIndex != Inits.size(); ++InitIndex) {
-    CXXCtorInitializer *Init = Inits[InitIndex];
-    const void *InitKey = GetKeyForMember(SemaRef.Context, Init);
+    const void *InitKey = GetKeyForMember(SemaRef.Context, Inits[InitIndex]);
 
     // Scan forward to try to find this initializer in the idealized
     // initializers list.
     for (; IdealIndex != NumIdealInits; ++IdealIndex)
       if (InitKey == IdealInitKeys[IdealIndex])
         break;
 
     // If we didn't find this initializer, it must be because we
     // scanned past it on a previous iteration.  That can only
     // happen if we're out of order;  emit a warning.
-    if (IdealIndex == NumIdealInits && PrevInit) {
-      Sema::SemaDiagnosticBuilder D =
-        SemaRef.Diag(PrevInit->getSourceLocation(),
-                     diag::warn_initializer_out_of_order);
-
-      if (PrevInit->isAnyMemberInitializer())
-        D << 0 << PrevInit->getAnyMember()->getDeclName();
-      else
-        D << 1 << PrevInit->getTypeSourceInfo()->getType();
-
-      if (Init->isAnyMemberInitializer())
-        D << 0 << Init->getAnyMember()->getDeclName();
-      else
-        D << 1 << Init->getTypeSourceInfo()->getType();
+    if (IdealIndex == NumIdealInits && InitIndex) {
+      WarnIndexes.push_back(InitIndex);
 
       // Move back to the initializer's location in the ideal list.
       for (IdealIndex = 0; IdealIndex != NumIdealInits; ++IdealIndex)
         if (InitKey == IdealInitKeys[IdealIndex])
           break;
 
       assert(IdealIndex < NumIdealInits &&
              "initializer not found in initializer list");
     }
+    CorrelatedInitOrder.emplace_back(IdealIndex, InitIndex);
+  }
 
-    PrevInit = Init;
+  if (WarnIndexes.empty())
+    return;
+
+  // Sort based on the ideal order, first in the pair.
+  llvm::sort(CorrelatedInitOrder,
+             [](auto &LHS, auto &RHS) { return LHS.first < RHS.first; });
+
+  // Introduce a new scope as SemaDiagnosticBuilder needs to be destroyed to
+  // emit the diagnostic before we can try adding notes.
+  {
+    Sema::SemaDiagnosticBuilder D = SemaRef.Diag(
+        Inits[WarnIndexes.front() - 1]->getSourceLocation(),
+        WarnIndexes.size() == 1 ? diag::warn_initializer_out_of_order
+                                : diag::warn_some_initializers_out_of_order);
+
+    for (unsigned I = 0; I < CorrelatedInitOrder.size(); ++I) {
+      if (CorrelatedInitOrder[I].second == I)
+        continue;
+      // Ideally we would be using InsertFromRange here, but clang doesn't
+      // appear to handle InsertFromRange correctly when the source range is
+      // modified by another fix-it.
+      D << FixItHint::CreateReplacement(
+          Inits[I]->getSourceRange(),
+          Lexer::getSourceText(
+              CharSourceRange::getTokenRange(
+                  Inits[CorrelatedInitOrder[I].second]->getSourceRange()),
+              SemaRef.getSourceManager(), SemaRef.getLangOpts()));
+    }
+
+    // If there is only 1 item out of order, the warning expects the name and
+    // type of each being added to it.
+    if (WarnIndexes.size() == 1) {
+      AddInitializerToDiag(D, Inits[WarnIndexes.front() - 1],
+                           Inits[WarnIndexes.front()]);
+      return;
+    }
+  }
+  // More than 1 item to warn, create notes letting the user know which ones
+  // are bad.
+  for (unsigned WarnIndex : WarnIndexes) {
+    const clang::CXXCtorInitializer *PrevInit = Inits[WarnIndex - 1];
+    auto D = SemaRef.Diag(PrevInit->getSourceLocation(),
+                          diag::note_initializer_out_of_order);
+    AddInitializerToDiag(D, PrevInit, Inits[WarnIndex]);
+    D << PrevInit->getSourceRange();
   }
 }
 
 namespace {
 bool CheckRedundantInit(Sema &S,
                         CXXCtorInitializer *Init,
                         CXXCtorInitializer *&PrevInit) {
   if (!PrevInit) {
while (Parent->isAnonymousStructOrUnion() || Parent->isUnion()) {
     }
 
     Child = Parent;
     Parent = cast<RecordDecl>(Parent->getDeclContext());
   }
 
   return false;
 }
-}
+} // namespace
 
 /// ActOnMemInitializers - Handle the member initializers for a constructor.
 void Sema::ActOnMemInitializers(Decl *ConstructorDecl,
                                 SourceLocation ColonLoc,
                                 ArrayRef<CXXCtorInitializer*> MemInits,
                                 bool AnyErrors) {
   if (!ConstructorDecl)
     return;

clang/test/FixIt/fixit-cxx-init-order.cpp

+// Due to the fix having multiple edits we can't use
+// '-fdiagnostics-parseable-fixits' to determine if fixes are correct. However,
+// running fixit recompile with 'Werror' should fail if the fixes are invalid.
+
+// RUN: %clang_cc1 %s -Werror=reorder-ctor -fixit-recompile -fixit-to-temporary
+// RUN: %clang_cc1 %s -Wreorder-ctor -verify -verify-ignore-unexpected=note
+
+struct Foo {
+  int A, B, C;
+
+  Foo() : A(1), B(3), C(2) {}
+  Foo(int) : A(1), C(2), B(3) {}      // expected-warning {{field 'C' will be initialized after field 'B'}}
+  Foo(unsigned) : C(2), B(3), A(1) {} // expected-warning {{initializer order does not match the declaration order}}
+};
+
+struct Bar : Foo {
+  int D, E, F;
+
+  Bar() : Foo(), D(1), E(2), F(3) {}
+  Bar(int) : D(1), E(2), F(3), Foo(4) {}      // expected-warning {{field 'F' will be initialized after base 'Foo'}}
+  Bar(unsigned) : F(3), E(2), D(1), Foo(4) {} // expected-warning {{initializer order does not match the declaration order}}
+};

clang/test/SemaCXX/constructor-initializer.cpp

 struct Base1 {};
 
 struct Derived : Base, Base1, virtual V {
   Derived ();
 };
 
 struct Current : Derived {
   int Derived;
-  Current() : Derived(1), ::Derived(), // expected-warning {{field 'Derived' will be initialized after base '::Derived'}} \
-                                       // expected-warning {{base class '::Derived' will be initialized after base 'Derived::V'}}
-                          ::Derived::Base(), // expected-error {{type '::Derived::Base' is not a direct or virtual base of 'Current'}}
-                           Derived::Base1(), // expected-error {{type 'Derived::Base1' is not a direct or virtual base of 'Current'}}
-                           Derived::V(),
-                           ::NonExisting(), // expected-error {{member initializer 'NonExisting' does not name a non-static data member or}}
-                           INT::NonExisting()  {} // expected-error {{'INT' (aka 'int') is not a class, namespace, or enumeration}} \
+  Current() : Derived(1), ::Derived(), // expected-warning {{initializer order does not match the declaration order}} \
+                                       // expected-note {{field 'Derived' will be initialized after base '::Derived'}} \
+                                       // expected-note {{base class '::Derived' will be initialized after base 'Derived::V'}}
+              ::Derived::Base(),       // expected-error {{type '::Derived::Base' is not a direct or virtual base of 'Current'}}
+              Derived::Base1(),        // expected-error {{type 'Derived::Base1' is not a direct or virtual base of 'Current'}}
+              ::NonExisting(),      // expected-error {{member initializer 'NonExisting' does not name a non-static data member or}}
+              INT::NonExisting() {} // expected-error {{'INT' (aka 'int') is not a class, namespace, or enumeration}} \
                                                   // expected-error {{member initializer 'NonExisting' does not name a non-static data member or}}
 };
 
 struct M {              // expected-note 2 {{candidate constructor (the implicit copy constructor)}}
 #if __cplusplus >= 201103L // C++11 or later
 // expected-note@-2 2 {{candidate constructor (the implicit move constructor) not viable}}
 #endif
 // expected-note@-4 2 {{'M' declared here}}

clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp

 // RUN: %clang_cc1  -fsyntax-only -Wreorder -verify %s
 
 struct BB {};
 
 struct BB1 {};
 
-class complex : public BB, BB1 { 
-public: 
+class complex : public BB, BB1 {
+public:
   complex()
-    : s2(1), // expected-warning {{field 's2' will be initialized after field 's1'}}
+      : s2(1), // expected-warning {{initializer order does not match the declaration order}} expected-note {{field 's2' will be initialized after field 's1'}}
         s1(1),
-      s3(3), // expected-warning {{field 's3' will be initialized after base 'BB1'}} 
-      BB1(), // expected-warning {{base class 'BB1' will be initialized after base 'BB'}}
-      BB()
-  {}
+        s3(3), // expected-note {{field 's3' will be initialized after base 'BB1'}}
+        BB1(), // expected-note {{base class 'BB1' will be initialized after base 'BB'}}
+        BB() {}
   int s1;
   int s2;
   int s3;
 }; 
 
 
 // testing virtual bases.