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