djasper added a comment.
Sorry, didn't have much time so far, will give this some more review later
today. One thing those is that the code could do with a bit more comments. It
would help reviewing (and future maintenance) of functions such as
reorderFieldsInConstructor to have brief comment (doesn't have to be more than
1-3 sentences) explaining what they are doing and maybe hint at how they are
doing it (not going into too much detail, there is the code for that).
================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:56
@@ +55,3 @@
+ Finder.matchAST(Context);
+ return Callback.RecordDecl;
+}
----------------
I think you can replace most of this function with:
auto Results = match(recordDecl(hasName(RecordName),
isDefinition()).bind("x"),
*Context.getTranslationUnitDecl(), Context));
if (Results.size() != 1) {
// error
}
return selectFirst<CXXRecordDecl>("x", Result);
Or make that a bit more elaborate with
================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:61
@@ +60,3 @@
+ ArrayRef<std::string> DesiredFieldsOrder,
+ SmallVector<unsigned, 4> &NewFieldsOrder) {
+ assert(Definition && "Definition is null");
----------------
I'd just return the NewFieldsOrder and let it return an empty vector in case of
an error. For boolean values, it is always hard to know whether they mean
"success" or "failure". And like this, it is unclear of what happens if
NewFieldsOrder isn't empty to start with (would run into the assert, but that's
not helpful).
================
Comment at: clang-reorder-fields/ReorderFieldsAction.h:29
@@ +28,3 @@
+ llvm::ArrayRef<std::string> DesiredFieldsOrder;
+ std::map<std::string, tooling::Replacements> &Replacements;
+
----------------
It's not obvious what this map is mapping. Add a comment here or at the
constructor.
Or actually just store a single Replacements object and use
groupReplacementsByFile (Replacement.h) when you need these by file.
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
