This adds a warning emitted by clang-reorder-fields when the reordering
fields breaks dependencies in the initializer list (such that
-Wuninitialized would warn). 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.
The patch also reformats a few lines that were over 80 columns wide.
Index: tools/extra/clang-reorder-fields/ReorderFieldsAction.cpp
===================================================================
--- tools/extra/clang-reorder-fields/ReorderFieldsAction.cpp (revision 306926)
+++ tools/extra/clang-reorder-fields/ReorderFieldsAction.cpp (working copy)
@@ -22,6 +22,7 @@
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h"
#include "clang/Tooling/Refactoring.h"
+#include "llvm/ADT/SetVector.h"
#include <algorithm>
#include <string>
@@ -28,6 +29,7 @@
namespace clang {
namespace reorder_fields {
using namespace clang::ast_matchers;
+using llvm::SmallSetVector;
/// \brief Finds the definition of a record by name.
///
@@ -127,13 +129,32 @@
return true;
}
+/// \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;
+ auto FoundExprs =
+ match(findAll(memberExpr(hasObjectExpression(cxxThisExpr())).bind("ME")),
+ *Initializer->getInit(), Context);
+ for (BoundNodes &BN : FoundExprs)
+ if (auto *MemExpr = BN.getNodeAs<MemberExpr>("ME"))
+ if (FieldDecl *FD = dyn_cast<FieldDecl>(MemExpr->getMemberDecl()))
+ Results.insert(FD);
+ return Results;
+}
+
/// \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)
@@ -140,8 +161,8 @@
return;
// The method FunctionDecl::isThisDeclarationADefinition returns false
- // for a defaulted function unless that function has been implicitly defined.
- // Thus this assert needs to be after the previous checks.
+ // for a defaulted function unless that function has been implicitly
+ // defined. Thus this assert needs to be after the previous checks.
assert(CtorDecl->isThisDeclarationADefinition() && "Not a definition");
SmallVector<unsigned, 10> NewFieldsPositions(NewFieldsOrder.size());
@@ -153,6 +174,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);
+ for (auto *UM : UsedMembers) {
+ const auto *ThisM = Initializer->getMember();
+ 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);
}
@@ -182,12 +219,12 @@
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 +236,9 @@
}
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;
}
Index: tools/extra/test/clang-reorder-fields/FieldDependencyWarning.cpp
===================================================================
--- tools/extra/test/clang-reorder-fields/FieldDependencyWarning.cpp (revision 0)
+++ tools/extra/test/clang-reorder-fields/FieldDependencyWarning.cpp (working copy)
@@ -0,0 +1,42 @@
+// RUN: clang-reorder-fields -record-name bar::Foo -fields-order y,z,c,x %s -- 2>&1 | FileCheck %s
+
+#include <string>
+
+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;
+}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
