[PATCH] D23279: clang-reorder-fields

[Daniel Jasper via cfe-commits]

djasper added a comment.

Probably the last round ..


================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:202
@@ +201,3 @@
+    for (const auto *C : RD->ctors()) {
+      if (C->isImplicit() || C->isDelegatingConstructor())
+        continue;
----------------
alexshap wrote:
> djasper wrote:
> > Why are you ruling out delegating constructors?
> ./include/clang/Basic/DiagnosticSemaKinds.td:1961:  "an initializer for a delegating constructor must appear alone";
> So i assumed that right now we don't need to do anything with them.
> Please, correct me if i am wrong. Anyway, good question.
I see. A comment about that wouldn't hurt.

Or, how about excluding all constructors with less than 2 initializers? That's intuitive, probably more efficient and includes delegating constructors :).

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:29
@@ +28,3 @@
+namespace clang {
+using namespace ast_matchers;
+
----------------
Put

  using namespace clang::ast_matchers;

into the namespace reorder_fields. Otherwise you are polluting the namespace clang.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:82
@@ +81,3 @@
+// FIXME: error-handling
+/// \brief Replaces one range of source code by another.
+static void
----------------
If this is what it is doing, maybe call it swapSourceRanges?

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:133
@@ +132,3 @@
+///
+/// A constructor can have initializers for an arbitrary subset of the classes
+/// fields.
----------------
- class's fields (or rephrase)
- no need to add a newline after the first sentence
- ".. aRE present"

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:177
@@ +176,3 @@
+/// \returns true on success
+static bool reorderFieldsInInitListExpr(
+    const InitListExpr *InitListEx, ArrayRef<unsigned> NewFieldsOrder,
----------------
While it's nice that this has a bool return value now, you aren't using it.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:182
@@ +181,3 @@
+  assert(InitListEx && "Init list expression is null");
+  if (!InitListEx->isExplicit() || !InitListEx->getNumInits())
+    return true;
----------------
Maybe instead getNumInits() <= 1?

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:231
@@ +230,3 @@
+    // We only need to reorder init list expressions for aggregate types.
+    // Now (v0) partial initialization is not supported.
+    // For other types the order of constructor parameters is used,
----------------
I'd move this sentence to the end of the comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D23279