[clang-tools-extra] r309505 - [clang-reorder-fields] Emit warning when reordering breaks member init list dependencies
Alexander Shaposhnikov via cfe-commits
cfe-commits at lists.llvm.org
Sat Jul 29 23:43:04 PDT 2017
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
More information about the cfe-commits
mailing list