njames93 created this revision.
njames93 added reviewers: rsmith, sammccall, aaron.ballman.
Herald added a subscriber: mgrang.
njames93 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D98745
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/FixIt/fixit-cxx-init-order.cpp
clang/test/SemaCXX/constructor-initializer.cpp
clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
Index: clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
===================================================================
--- clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
+++ clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
@@ -7,10 +7,10 @@
class complex : public BB, BB1 {
public:
complex()
- : s2(1), // expected-warning {{field 's2' will be initialized after field 's1'}}
+ : 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-warning {{field 's3' will be initialized after base 'BB1'}}
- BB1(), // expected-warning {{base class 'BB1' will be initialized after base '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;
Index: clang/test/SemaCXX/constructor-initializer.cpp
===================================================================
--- clang/test/SemaCXX/constructor-initializer.cpp
+++ clang/test/SemaCXX/constructor-initializer.cpp
@@ -91,8 +91,9 @@
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'}}
+ 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(),
Index: clang/test/FixIt/fixit-cxx-init-order.cpp
===================================================================
--- /dev/null
+++ 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}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===================================================================
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -5239,6 +5239,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<CXXCtorInitializer *> Inits) {
@@ -5288,10 +5302,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<unsigned> WarnIndexes;
+ // Correllates 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.
@@ -5302,20 +5321,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)
@@ -5325,8 +5332,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();
}
}
@@ -5394,7 +5447,7 @@
return false;
}
-}
+} // namespace
/// ActOnMemInitializers - Handle the member initializers for a constructor.
void Sema::ActOnMemInitializers(Decl *ConstructorDecl,
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8603,6 +8603,15 @@
"%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<
+ "some initializers aren't given in the correct 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">,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
