[PATCH] D23279: clang-reorder-fields

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:29
@@ +28,3 @@
+using namespace llvm;
+using namespace clang;
+using namespace clang::ast_matchers;
----------------
Put everything here into the namespace clang and remove "using namespace clang". Similarly check if you (then) really still need the "using namespace llvm".

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:92
@@ +91,3 @@
+
+/// \brief Reorders fields in the definition of a struct/class
+/// At the moment reodering of fields with 
----------------
You need an empty line in the comment to end the "\brief", I think.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:108
@@ +107,3 @@
+      continue;
+    assert(Field->getAccess() ==
+               Fields[NewFieldsOrder[FieldIndex]]->getAccess() &&
----------------
I think you need to properly handle this in a different way, e.g. returning a bool value and aborting the replacement operation. Two main reasons:
- Otherwise you cannot test it (well)
- This alone mean that if I build this tool in a release (non-assert-enabled) build, it will just do the wrong thing.

asserts are supposed to be used only for code paths that you don't expect to ever get into.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:118
@@ +117,3 @@
+
+/// \brief Reorders initializers in a C++ struct/class constructor
+static void reorderFieldsInConstructor(
----------------
This is not what I meant be writing a comment. I consider this comment pretty much useless, it doesn't add any information on top of the function name. You need to write significantly more comments so people can actually (easily) understand what your implementation is doing. You can write a sentence or two up here and/or add comments to the different things that are done within the function. Without that, this is really hard to follow, e.g. the different between NewFieldsOrder and NewFieldsPositions is not at all intuitive (at least to me). You want to write enough comments that other people can at a quick glance understand a) what this is doing and b) that the implementation is doing what you are writing.

E.g. something like:
  // A constructor can have initializers for an arbitrary subset of the classes fields. Thus,
  // we need to ensure that we reorder just the initializers that a present.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:148
@@ +147,3 @@
+         NewWrittenInitializersOrder.size());
+  for (unsigned I = 0, E = NewWrittenInitializersOrder.size(); I < E; ++I)
+    if (OldWrittenInitializersOrder[I] != NewWrittenInitializersOrder[I])
----------------
By convention we use I and E for iterators, i and e for ints.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:165
@@ +164,3 @@
+    return;
+  assert(InitListEx->getNumInits() == NewFieldsOrder.size() &&
+         "Currently only full initialization is supported");
----------------
Same here, an assert is insufficient.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:202
@@ +201,3 @@
+    for (const auto *C : RD->ctors()) {
+      if (C->isImplicit() || C->isDelegatingConstructor())
+        continue;
----------------
Why are you ruling out delegating constructors?

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:204
@@ +203,3 @@
+        continue;
+      if (const auto *D = C->getDefinition())
+        reorderFieldsInConstructor(cast<const CXXConstructorDecl>(D),
----------------
I'd do:

  if (const auto *D = dyn_cast<CXXConstructorDecl>(C->getDefinition))
    reorderFieldsInConstructor(D, NewFieldsOrder, Context, Replacements);

But both should be fine.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:208
@@ +207,3 @@
+    }
+    if (!RD->isAggregate())
+      return;
----------------
Here you could add a comment like:

  // We only need to reorder init list expressions for aggregate types. For other types
  // the order of constructor parameters is used, which we don't change.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:210
@@ +209,3 @@
+      return;
+    for (auto Result : match(initListExpr().bind("initListExpr"), Context)) {
+      const auto *E = Result.getNodeAs<InitListExpr>("initListExpr");
----------------
Something like:

  match(initListExpr(hasType(equalsNode(RD)))

should work.

================
Comment at: clang-reorder-fields/tool/ClangReorderFields.cpp:68
@@ +67,3 @@
+
+  int ExitCode = Tool.run(Factory.get());
+  LangOptions DefaultLangOptions;
----------------
Should you continue if the exit code isn't 0 here?


Repository:
  rL LLVM

https://reviews.llvm.org/D23279