[PATCH] D35972: Add warning to clang-reorder-fields when reordering breaks member init list dependencies

[Sam Conrad via Phabricator via cfe-commits]

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 <string>
+
----------------
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