sameconrad updated this revision to Diff 108812.
sameconrad added a comment.
Add additional tests for cases with base and derived classes.
Add clarifying comments about emitting warnings for base class members during
reordering checks.
Add fix for bug where reordering in init lists that contain non-member
initializers causes segfault.
https://reviews.llvm.org/D35972
Files:
clang-reorder-fields/ReorderFieldsAction.cpp
test/clang-reorder-fields/ClassDerived.cpp
test/clang-reorder-fields/FieldDependencyWarning.cpp
test/clang-reorder-fields/FieldDependencyWarningDerived.cpp
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<char>(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
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/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<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
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 <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.
///
@@ -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<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
@@ -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<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 +175,24 @@
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()]) {
+ 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);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
