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