diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -8606,6 +8606,15 @@ "%select{field|base class}0 %1 will be initialized after " "%select{field|base}2 %3">, InGroup, DefaultIgnore; + +def warn_some_initializers_out_of_order : Warning< + "some initializers aren't given in the correct order">, + InGroup, 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">, diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/lib/Sema/SemaDeclCXX.cpp @@ -5235,6 +5235,20 @@ return Member->getAnyMember()->getCanonicalDecl(); } +static void AddInitializerToDiag(const Sema::SemaDiagnosticBuilder &Diag, + const CXXCtorInitializer *Previous, + const CXXCtorInitializer *Current) { + if (Previous->isAnyMemberInitializer()) + Diag << 0 << Previous->getAnyMember()->getDeclName(); + else + Diag << 1 << Previous->getTypeSourceInfo()->getType(); + + if (Current->isAnyMemberInitializer()) + Diag << 0 << Current->getAnyMember()->getDeclName(); + else + Diag << 1 << Current->getTypeSourceInfo()->getType(); +} + static void DiagnoseBaseOrMemInitializerOrder( Sema &SemaRef, const CXXConstructorDecl *Constructor, ArrayRef Inits) { @@ -5284,10 +5298,15 @@ 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 WarnIndexes; + // Correllates the index of an initializer in the init-list to the index of + // the field/base in the class. + SmallVector, 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. @@ -5298,20 +5317,8 @@ // 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) @@ -5321,8 +5328,54 @@ 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(); } } @@ -5390,7 +5443,7 @@ return false; } -} +} // namespace /// ActOnMemInitializers - Handle the member initializers for a constructor. void Sema::ActOnMemInitializers(Decl *ConstructorDecl, diff --git a/clang/test/FixIt/fixit-cxx-init-order.cpp b/clang/test/FixIt/fixit-cxx-init-order.cpp new file mode 100644 --- /dev/null +++ b/clang/test/FixIt/fixit-cxx-init-order.cpp @@ -0,0 +1,22 @@ +// 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 {{some initializers aren't given in the correct 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 {{some initializers aren't given in the correct order}} +}; diff --git a/clang/test/SemaCXX/constructor-initializer.cpp b/clang/test/SemaCXX/constructor-initializer.cpp --- a/clang/test/SemaCXX/constructor-initializer.cpp +++ b/clang/test/SemaCXX/constructor-initializer.cpp @@ -91,13 +91,14 @@ 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 {{some initializers aren't given in the correct 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'}} + 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}} \ // expected-error {{member initializer 'NonExisting' does not name a non-static data member or}} }; diff --git a/clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp b/clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp --- a/clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp +++ b/clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp @@ -5,14 +5,13 @@ struct BB1 {}; class complex : public BB, BB1 { -public: +public: complex() - : s2(1), // expected-warning {{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() - {} + : s2(1), // expected-warning {{some initializers aren't given in the correct order}} expected-note {{field 's2' will be initialized after field 's1'}} + s1(1), + 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;