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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
