Author: alexshap Date: Sat Jul 29 23:43:03 2017 New Revision: 309505 URL: http://llvm.org/viewvc/llvm-project?rev=309505&view=rev Log: [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 Added: clang-tools-extra/trunk/test/clang-reorder-fields/ClassDerived.cpp clang-tools-extra/trunk/test/clang-reorder-fields/FieldDependencyWarning.cpp clang-tools-extra/trunk/test/clang-reorder-fields/FieldDependencyWarningDerived.cpp Modified: clang-tools-extra/trunk/clang-reorder-fields/ReorderFieldsAction.cpp Modified: clang-tools-extra/trunk/clang-reorder-fields/ReorderFieldsAction.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-reorder-fields/ReorderFieldsAction.cpp?rev=309505&r1=309504&r2=309505&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-reorder-fields/ReorderFieldsAction.cpp (original) +++ clang-tools-extra/trunk/clang-reorder-fields/ReorderFieldsAction.cpp Sat Jul 29 23:43:03 2017 @@ -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 <algorithm> #include <string> 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, SourceRa 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<FieldDecl *, 1> +findMembersUsedInInitExpr(const CXXCtorInitializer *Initializer, + ASTContext &Context) { + SmallSetVector<FieldDecl *, 1> 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<MemberExpr>("ME")) + if (auto *FD = dyn_cast<FieldDecl>(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<unsigned> NewFieldsOrder, - const ASTContext &Context, + ASTContext &Context, std::map<std::string, tooling::Replacements> &Replacements) { assert(CtorDecl && "Constructor declaration is null"); if (CtorDecl->isImplicit() || CtorDecl->getNumCtorInitializers() <= 1) @@ -151,8 +176,26 @@ static void reorderFieldsInConstructor( SmallVector<const CXXCtorInitializer *, 10> OldWrittenInitializersOrder; SmallVector<const CXXCtorInitializer *, 10> 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<std::string, tooling::Replacements> &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<CXXConstructorDecl>(C->getDefinition())) reorderFieldsInConstructor(cast<const CXXConstructorDecl>(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. Added: clang-tools-extra/trunk/test/clang-reorder-fields/ClassDerived.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-reorder-fields/ClassDerived.cpp?rev=309505&view=auto ============================================================================== --- clang-tools-extra/trunk/test/clang-reorder-fields/ClassDerived.cpp (added) +++ clang-tools-extra/trunk/test/clang-reorder-fields/ClassDerived.cpp Sat Jul 29 23:43:03 2017 @@ -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<char>\(ny\)\),}} + z(static_cast<char>(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 Added: clang-tools-extra/trunk/test/clang-reorder-fields/FieldDependencyWarning.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-reorder-fields/FieldDependencyWarning.cpp?rev=309505&view=auto ============================================================================== --- clang-tools-extra/trunk/test/clang-reorder-fields/FieldDependencyWarning.cpp (added) +++ clang-tools-extra/trunk/test/clang-reorder-fields/FieldDependencyWarning.cpp Sat Jul 29 23:43:03 2017 @@ -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; +} Added: clang-tools-extra/trunk/test/clang-reorder-fields/FieldDependencyWarningDerived.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-reorder-fields/FieldDependencyWarningDerived.cpp?rev=309505&view=auto ============================================================================== --- clang-tools-extra/trunk/test/clang-reorder-fields/FieldDependencyWarningDerived.cpp (added) +++ clang-tools-extra/trunk/test/clang-reorder-fields/FieldDependencyWarningDerived.cpp Sat Jul 29 23:43:03 2017 @@ -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<char>(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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
