From b687fdda29085c9f9e4925be38b7cac97f871c99 Mon Sep 17 00:00:00 2001 From: Alexander Shaposhnikov Date: Sun, 30 Jul 2017 06:43:03 +0000 Subject: [PATCH] [clang-reorder-fields] Emit warning when reordering breaks member init list dependencies This diff adds a warning emitted by clang-reorder-fields when reordering breaks dependencies in the initializer list. Patch by Sam Conrad! Differential revision: https://reviews.llvm.org/D35972 llvm-svn: 309505 --- .../clang-reorder-fields/ReorderFieldsAction.cpp | 74 +++++++++++++++++----- .../test/clang-reorder-fields/ClassDerived.cpp | 33 ++++++++++ .../FieldDependencyWarning.cpp | 54 ++++++++++++++++ .../FieldDependencyWarningDerived.cpp | 36 +++++++++++ 4 files changed, 181 insertions(+), 16 deletions(-) create mode 100644 clang-tools-extra/test/clang-reorder-fields/ClassDerived.cpp create mode 100644 clang-tools-extra/test/clang-reorder-fields/FieldDependencyWarning.cpp create mode 100644 clang-tools-extra/test/clang-reorder-fields/FieldDependencyWarningDerived.cpp diff --git a/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp b/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp index ae22b34..7cb8abe 100644 --- a/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp +++ b/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp @@ -22,21 +22,23 @@ #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. /// /// \returns nullptr if the name is ambiguous or not found. static const RecordDecl *findDefinition(StringRef RecordName, ASTContext &Context) { - auto Results = match( - recordDecl(hasName(RecordName), isDefinition()).bind("recordDecl"), - Context); + auto Results = + match(recordDecl(hasName(RecordName), isDefinition()).bind("recordDecl"), + Context); if (Results.empty()) { llvm::errs() << "Definition of " << RecordName << " not found\n"; return nullptr; @@ -91,6 +93,28 @@ addReplacement(SourceRange Old, SourceRange New, const ASTContext &Context, 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 @@ -129,11 +153,12 @@ static bool reorderFieldsInDefinition( /// \brief Reorders initializers in a C++ struct/class constructor. /// -/// A constructor can have initializers for an arbitrary subset of the class's fields. -/// Thus, we need to ensure that we reorder just the initializers that are present. +/// A constructor can have initializers for an arbitrary subset of the class's +/// fields. 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 +176,26 @@ static void reorderFieldsInConstructor( 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()]) { + DiagnosticsEngine &DiagEngine = Context.getDiagnostics(); + auto Description = ("reordering field " + UM->getName() + " after " + + ThisM->getName() + " makes " + UM->getName() + + " uninitialized when used in init expression") + .str(); + unsigned ID = DiagEngine.getDiagnosticIDs()->getCustomDiagID( + DiagnosticIDs::Warning, Description); + DiagEngine.Report(Initializer->getSourceLocation(), ID); + } + } + OldWrittenInitializersOrder.push_back(Initializer); NewWrittenInitializersOrder.push_back(Initializer); } @@ -182,12 +225,12 @@ static bool reorderFieldsInInitListExpr( const ASTContext &Context, std::map &Replacements) { assert(InitListEx && "Init list expression is null"); - // We care only about InitListExprs which originate from source code. + // We care only about InitListExprs which originate from source code. // Implicit InitListExprs are created by the semantic analyzer. if (!InitListEx->isExplicit()) return true; - // The method InitListExpr::getSyntacticForm may return nullptr indicating that - // the current initializer list also serves as its syntactic form. + // The method InitListExpr::getSyntacticForm may return nullptr indicating + // that the current initializer list also serves as its syntactic form. if (const auto *SyntacticForm = InitListEx->getSyntacticForm()) InitListEx = SyntacticForm; // If there are no initializers we do not need to change anything. @@ -199,10 +242,9 @@ static bool reorderFieldsInInitListExpr( } for (unsigned i = 0, e = InitListEx->getNumInits(); i < e; ++i) if (i != NewFieldsOrder[i]) - addReplacement( - InitListEx->getInit(i)->getSourceRange(), - InitListEx->getInit(NewFieldsOrder[i])->getSourceRange(), Context, - Replacements); + addReplacement(InitListEx->getInit(i)->getSourceRange(), + InitListEx->getInit(NewFieldsOrder[i])->getSourceRange(), + Context, Replacements); return true; } @@ -239,9 +281,9 @@ public: for (const auto *C : CXXRD->ctors()) if (const auto *D = dyn_cast(C->getDefinition())) reorderFieldsInConstructor(cast(D), - NewFieldsOrder, Context, Replacements); + NewFieldsOrder, Context, Replacements); - // We only need to reorder init list expressions for + // We only need to reorder init list expressions for // plain C structs or C++ aggregate types. // For other types the order of constructor parameters is used, // which we don't change at the moment. diff --git a/clang-tools-extra/test/clang-reorder-fields/ClassDerived.cpp b/clang-tools-extra/test/clang-reorder-fields/ClassDerived.cpp new file mode 100644 index 0000000..e7e9ac1 --- /dev/null +++ b/clang-tools-extra/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 diff --git a/clang-tools-extra/test/clang-reorder-fields/FieldDependencyWarning.cpp b/clang-tools-extra/test/clang-reorder-fields/FieldDependencyWarning.cpp new file mode 100644 index 0000000..bbe0e7f --- /dev/null +++ b/clang-tools-extra/test/clang-reorder-fields/FieldDependencyWarning.cpp @@ -0,0 +1,54 @@ +// RUN: clang-reorder-fields -record-name bar::Foo -fields-order y,z,c,x %s -- 2>&1 | FileCheck --check-prefix=CHECK-MESSAGES %s +// FIXME: clang-reorder-fields should provide -verify mode to make writing these checks +// easier and more accurate, for now we follow clang-tidy's approach. + +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-MESSAGES: :[[@LINE-1]]:3: warning: reordering field x after z makes x uninitialized when used in init expression + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: reordering field c after z makes c uninitialized when used in init expression +{} + +Foo::Foo(int nx) : + x(nx), + y(x), + c(0), + z(bar(bar(x)), c) + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: reordering field x after y makes x uninitialized when used in init expression + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: reordering field x after z makes x uninitialized when used in init expression + // CHECK-MESSAGES: :[[@LINE-3]]:3: 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; +} diff --git a/clang-tools-extra/test/clang-reorder-fields/FieldDependencyWarningDerived.cpp b/clang-tools-extra/test/clang-reorder-fields/FieldDependencyWarningDerived.cpp new file mode 100644 index 0000000..bbc58ae --- /dev/null +++ b/clang-tools-extra/test/clang-reorder-fields/FieldDependencyWarningDerived.cpp @@ -0,0 +1,36 @@ +// RUN: clang-reorder-fields -record-name bar::Derived -fields-order z,y %s -- 2>&1 | FileCheck --check-prefix=CHECK-MESSAGES %s +// FIXME: clang-reorder-fields should provide -verify mode to make writing these checks +// easier and more accurate, for now we follow clang-tidy's approach. + +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-MESSAGES: :[[@LINE-1]]:3: 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-MESSAGES-NOT: :[[@LINE-1]]:3: warning: reordering field x after z makes x uninitialized when used in init expression +{} + +} // namespace bar -- 2.7.4