Index: clang-reorder-fields/ReorderFieldsAction.cpp =================================================================== --- clang-reorder-fields/ReorderFieldsAction.cpp +++ clang-reorder-fields/ReorderFieldsAction.cpp @@ -22,12 +22,14 @@ #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/Lexer.h" #include "clang/Tooling/Refactoring.h" +#include "llvm/ADT/SetVector.h" #include #include namespace clang { namespace reorder_fields { using namespace clang::ast_matchers; +using llvm::SmallSetVector; /// \brief Finds the definition of a record by name. /// @@ -91,6 +93,28 @@ consumeError(Replacements[R.getFilePath()].add(R)); } +/// \brief Find all member fields used in the given init-list initializer expr +/// that belong to the same record +/// +/// \returns a set of field declarations, empty if none were present +static SmallSetVector +findMembersUsedInInitExpr(const CXXCtorInitializer *Initializer, + ASTContext &Context) { + SmallSetVector Results; + // Note that this does not pick up member fields of base classes since + // for those accesses Sema::PerformObjectMemberConversion always inserts an + // UncheckedDerivedToBase ImplicitCastExpr between the this expr and the + // object expression + auto FoundExprs = + match(findAll(memberExpr(hasObjectExpression(cxxThisExpr())).bind("ME")), + *Initializer->getInit(), Context); + for (BoundNodes &BN : FoundExprs) + if (auto *MemExpr = BN.getNodeAs("ME")) + if (auto *FD = dyn_cast(MemExpr->getMemberDecl())) + Results.insert(FD); + return Results; +} + /// \brief Reorders fields in the definition of a struct/class. /// /// At the moment reodering of fields with @@ -133,7 +157,7 @@ /// Thus, we need to ensure that we reorder just the initializers that are present. static void reorderFieldsInConstructor( const CXXConstructorDecl *CtorDecl, ArrayRef NewFieldsOrder, - const ASTContext &Context, + ASTContext &Context, std::map &Replacements) { assert(CtorDecl && "Constructor declaration is null"); if (CtorDecl->isImplicit() || CtorDecl->getNumCtorInitializers() <= 1) @@ -151,8 +175,24 @@ SmallVector OldWrittenInitializersOrder; SmallVector NewWrittenInitializersOrder; for (const auto *Initializer : CtorDecl->inits()) { - if (!Initializer->isWritten()) + if (!Initializer->isMemberInitializer() || !Initializer->isWritten()) continue; + + // Warn if this reordering violates initialization expr dependencies + const FieldDecl *ThisM = Initializer->getMember(); + const auto UsedMembers = findMembersUsedInInitExpr(Initializer, Context); + for (const FieldDecl *UM : UsedMembers) { + if (NewFieldsPositions[UM->getFieldIndex()] > + NewFieldsPositions[ThisM->getFieldIndex()]) { + llvm::errs() << Context.getSourceManager().getSpellingLineNumber( + Initializer->getSourceLocation()) + << ": Warning: reordering field " << UM->getName() + << " after " << ThisM->getName() << " makes " + << UM->getName() + << " uninitialized when used in init expression\n"; + } + } + OldWrittenInitializersOrder.push_back(Initializer); NewWrittenInitializersOrder.push_back(Initializer); } Index: test/clang-reorder-fields/ClassDerived.cpp =================================================================== --- test/clang-reorder-fields/ClassDerived.cpp +++ test/clang-reorder-fields/ClassDerived.cpp @@ -0,0 +1,33 @@ +// RUN: clang-reorder-fields -record-name bar::Derived -fields-order z,y %s -- | FileCheck %s + +namespace bar { +class Base { +public: + Base(int nx, int np) : x(nx), p(np) {} + int x; + int p; +}; + + +class Derived : public Base { +public: + Derived(long ny); + Derived(char nz); +private: + long y; + char z; +}; + +Derived::Derived(long ny) : + Base(ny, 0), + y(ny), // CHECK: {{^ z\(static_cast\(ny\)\),}} + z(static_cast(ny)) // CHECK-NEXT: {{^ y\(ny\)}} +{} + +Derived::Derived(char nz) : + Base(1, 2), + y(nz), // CHECK: {{^ z\(x\),}} + z(x) // CHECK-NEXT: {{^ y\(nz\)}} +{} + +} // namespace bar Index: test/clang-reorder-fields/FieldDependencyWarning.cpp =================================================================== --- test/clang-reorder-fields/FieldDependencyWarning.cpp +++ test/clang-reorder-fields/FieldDependencyWarning.cpp @@ -0,0 +1,49 @@ +// RUN: clang-reorder-fields -record-name bar::Foo -fields-order y,z,c,x %s -- 2>&1 | FileCheck %s + +namespace bar { + +struct Dummy { + Dummy(int x, char c) : x(x), c(c) {} + int x; + char c; +}; + +class Foo { +public: + Foo(int x, double y, char cin); + Foo(int nx); + Foo(); + int x; + double y; + char c; + Dummy z; +}; + +static char bar(char c) { + return c + 1; +} + +Foo::Foo() : x(), y(), c(), z(0, 'a') {} + +Foo::Foo(int x, double y, char cin) : + x(x), + y(y), + c(cin), + z(this->x, bar(c)) // CHECK: [[@LINE]]: Warning: reordering field x after z makes x uninitialized when used in init expression + // CHECK-NEXT: [[@LINE-1]]: Warning: reordering field c after z makes c uninitialized when used in init expression +{} + +Foo::Foo(int nx) : + x(nx), + y(x), // CHECK-NEXT: [[@LINE]]: Warning: reordering field x after y makes x uninitialized when used in init expression + c(0), + z(bar(bar(x)), c) // CHECK-NEXT: [[@LINE]]: Warning: reordering field x after z makes x uninitialized when used in init expression + // CHECK-NEXT: [[@LINE-1]]: Warning: reordering field c after z makes c uninitialized when used in init expression +{} + +} // namespace bar + +int main() { + bar::Foo F(5, 12.8, 'c'); + return 0; +} Index: test/clang-reorder-fields/FieldDependencyWarningDerived.cpp =================================================================== --- test/clang-reorder-fields/FieldDependencyWarningDerived.cpp +++ test/clang-reorder-fields/FieldDependencyWarningDerived.cpp @@ -0,0 +1,34 @@ +// RUN: clang-reorder-fields -record-name bar::Derived -fields-order z,y %s -- 2>&1 | FileCheck %s + +namespace bar { +struct Base { + int x; + int p; +}; + + +class Derived : public Base { +public: + Derived(long ny); + Derived(char nz); +private: + long y; + char z; +}; + +Derived::Derived(long ny) : + Base(), + y(ny), + z(static_cast(y)) // CHECK: [[@LINE]]: Warning: reordering field y after z makes y uninitialized when used in init expression +{} + +Derived::Derived(char nz) : + Base(), + y(nz), + // Check that base class fields are correctly ignored in reordering checks + // x has field index 1 and so would improperly warn if this wasn't the case since the command for this file swaps field indexes 1 and 2 + z(x) // CHECK-NOT: [[@LINE]]: Warning: reordering field x after z makes x uninitialized when used in init expression + +{} + +} // namespace bar