[PATCH] D23279: clang-reorder-fields

[Daniel Jasper via cfe-commits]

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