[PATCH] D23279: clang-reorder-fields

[Alexander Shaposhnikov via cfe-commits]

alexshap added a comment.

> My only high-level comment so far (will try to review in more detail later) is: I find it strange to mix ASTMatchers and a >RecursiveASTVisitor in the same tool. It seem that if you are using ASTMatchers internally for other things, you should also >use ASTMatchers to find all the constructors etc. That should (I hope) make the code a bit simpler.


i can use matchers (and actually i did that  originally) for everything but can try to explain the rationale  behind my (current) approach:
Matcher is used for the lookup of the definition of the record by name, the list of ctors is extracted from the definition (Defintion->ctors()) and doesn't require any extra AST traverses. However to handle aggregates and unified initialization we need to traverse the AST and find all the call-sites. It's easy to do it with matchers as well however i was keeping in mind the future potential features (like changing the order of constructor arguments) - in that case one needs just to add another Visit* method to ReorderingASTVisitor instead of matching the full AST against the new Matcher (so i would expect it be faster and the code probably will be more concise).


Repository:
  rL LLVM

https://reviews.llvm.org/D23279