[PATCH] D35972: Add warning to clang-reorder-fields when reordering breaks member init list dependencies
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(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\(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: 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)
[PATCH] D35972: Add warning to clang-reorder-fields when reordering breaks member init list dependencies
sameconrad added inline comments.
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:178
for (const auto *Initializer : CtorDecl->inits()) {
-if (!Initializer->isWritten())
+if (!Initializer->isMemberInitializer() || !Initializer->isWritten())
continue;
While adding tests for emitting warnings about base class members I found
another bug here- non-member initializers don't have a field decl and so would
result in undefind behavior further down when accessing field indexes (I added
the test case ClassDerived which would cause a segfault before this change).
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:111
+Results.insert(FD);
+ return Results;
+}
compnerd wrote:
> What if the `FieldDecl` belongs to a base class? Can you add a test case for
> that scenario?
I've added tests for this scenario and an explanatory comment. It doesn't seem
this is actually a problem due to the way Clang constructs the AST.
Sema::PerformObjectMemberConversion always inserts an implicit
UncheckedDerivedToBase cast when accessing a base class member which means that
hasObjectExpression(cxxThisExpr()) won't actually match against such an
expression.
Comment at: test/clang-reorder-fields/FieldDependencyWarning.cpp:3
+
+#include
+
alexshap wrote:
> tests should not depend on STL (for good examples see how things are done in
> clang-tidy tests), so simply remove this include and define a small class
> with a constructor right here
This has been updated to use a simple struct.
https://reviews.llvm.org/D35972
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35972: Add warning to clang-reorder-fields when reordering breaks member init list dependencies
sameconrad added a comment. In https://reviews.llvm.org/D35972#825348, @alexshap wrote: > LGTM, thanks! > do you have commit access ? (if not i can commit this patch for you) I don't have commit access, so you will have to commit it. Thank you https://reviews.llvm.org/D35972 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35972: Add warning to clang-reorder-fields when reordering breaks member init list dependencies
sameconrad created this revision.
sameconrad added a project: clang-tools-extra.
This adds a warning emitted by clang-reorder-fields when the reordering fields
breaks dependencies in the initializer list (such that -Wuninitialized would
warn when compiling). For example, given:
Foo::Foo(int x)
: a(x)
, b(a) {}
Reordering fields to [b,a] gives:
Foo::Foo(int x)
: b(a)
, a(x) {}
Emits the warning:
2: Warning: reordering field a after b makes a uninitialized when used in init
expression.
https://reviews.llvm.org/D35972
Files:
clang-reorder-fields/ReorderFieldsAction.cpp
test/clang-reorder-fields/FieldDependencyWarning.cpp
Index: test/clang-reorder-fields/FieldDependencyWarning.cpp
===
--- test/clang-reorder-fields/FieldDependencyWarning.cpp
+++ test/clang-reorder-fields/FieldDependencyWarning.cpp
@@ -0,0 +1,42 @@
+// RUN: clang-reorder-fields -record-name bar::Foo -fields-order y,z,c,x %s -- 2>&1 | FileCheck %s
+
+#include
+
+namespace bar {
+class Foo {
+public:
+ Foo(int x, double y, char cin);
+ Foo(int nx);
+ Foo();
+ int x;
+ double y;
+ char c;
+ std::string z;
+};
+
+static char bar(char c) {
+ return c + 1;
+}
+
+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: 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,24 @@
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;
+ 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 +153,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)
@@ -153,6 +173,22 @@
for (const auto *Initializer : CtorDecl->inits()) {
if (!Initializer->isWritten())
continue;
+
+// Warn if this reordering violates initialization expr dependencies
+const auto UsedMembers = findMembersUsedInInitExpr(Initializer, Context);
+const FieldDecl *ThisM = Initializer->getMember();
+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 expre
