[PATCH] D23279: clang-reorder-fields

[Daniel Jasper via cfe-commits]

djasper added a comment.

I am serious about writing more comments.


================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:41
@@ +40,3 @@
+  }
+  if (Results.size() != 1) {
+    errs() << "The name " << RecordName
----------------
Make this "> 1" instead of "!= 1". Not really important, but imagine that at some point for some reason you wanted to change the order of the two ifs.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:137
@@ +136,3 @@
+            std::end(NewWrittenInitializersOrder), ByFieldNewPosition);
+  assert(OldWrittenInitializersOrder.size() ==
+         NewWrittenInitializersOrder.size());
----------------
This assert seems pretty useless.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:139
@@ +138,3 @@
+         NewWrittenInitializersOrder.size());
+  for (unsigned Index = 0; Index < NewWrittenInitializersOrder.size(); ++Index)
+    addReplacement(OldWrittenInitializersOrder[Index]->getSourceRange(),
----------------
Also use "i" and "e" as below.

And you shouldn't be creating a replacement if new order == old order.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:152
@@ +151,3 @@
+  // SourceLocation::isValid is const.
+  if (!const_cast<clang::InitListExpr *>(InitListEx)->isExplicit() ||
+      !InitListEx->getNumInits())
----------------
I think it is an oversight that isExplicit() isn't const. Can you send out a separate patch to fix that instead of using const_cast here? Seems horrible ;)

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:157
@@ +156,3 @@
+         "Currently only full initialization is supported");
+  for (unsigned Index = 0, NumInits = InitListEx->getNumInits();
+       Index < NumInits; ++Index) {
----------------
Use "i" and "e" instead of "Index" and "NumInits". That's a common pattern used in a lot of places.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:197
@@ +196,3 @@
+        continue;
+      const auto *D = C->getDefinition();
+      if (!D)
----------------
I'd write this as:

  if (const auto *D = C->getDefinition())
    reorderFieldsInConstructor(...);

================
Comment at: clang-reorder-fields/ReorderFieldsAction.h:31
@@ +30,3 @@
+
+public:
+  ReorderFieldsAction(
----------------
I don't why clang-rename does what it does at the moment. My main point is that we have already implemented splitting up replacements by file in groupByReplacementsByFile. No need to reimplement that here or make the interface of this function more complex than it needs to be.

================
Comment at: clang-reorder-fields/tool/ClangReorderFields.cpp:44
@@ +43,3 @@
+  bool parse(cl::Option &O, StringRef ArgName, const std::string &ArgValue,
+             SmallVector<std::string, 4> &Val) {
+    Val.clear();
----------------
Don't use and output parameter. Just return Parts.

================
Comment at: clang-reorder-fields/tool/ClangReorderFields.cpp:62
@@ +61,3 @@
+
+static cl::opt<bool> Inplace("i", cl::desc("Overwrite edited <file>s."),
+                             cl::cat(ClangReorderFieldsCategory));
----------------
remove <>, just "files".

================
Comment at: clang-reorder-fields/tool/ClangReorderFields.cpp:74
@@ +73,3 @@
+
+  reorder_fields::ReorderFieldsAction Action(RecordName, FieldsOrder,
+                                             Tool.getReplacements());
----------------
Should you be checking whether all the commandline flags are given and display a usage message if not here?

================
Comment at: clang-reorder-fields/tool/ClangReorderFields.cpp:81
@@ +80,3 @@
+  if (Inplace) {
+    ExitCode = Tool.runAndSave(Factory.get());
+  } else {
----------------
just:

  return Tool.runAndSave(..);

Here and elsewhere.


Repository:
  rL LLVM

https://reviews.llvm.org/D23279