[PATCH] D24754: [cleanup] Remove excessive padding from RedeclarableResult

[Alexander Shaposhnikov via cfe-commits]

alexshap added inline comments.

================
Comment at: lib/Serialization/ASTReaderDecl.cpp:154
@@ -153,3 +153,3 @@
     public:
-      RedeclarableResult(GlobalDeclID FirstID, Decl *MergeWith, bool IsKeyDecl)
-          : FirstID(FirstID), MergeWith(MergeWith), IsKeyDecl(IsKeyDecl) {}
+      RedeclarableResult(Decl *MergeWith, GlobalDeclID FirstID, bool IsKeyDecl)
+          : MergeWith(MergeWith), FirstID(FirstID), IsKeyDecl(IsKeyDecl) {}
----------------
vsk wrote:
> alexshap wrote:
> > vsk wrote:
> > > Why do you need to change the order of the parameters in the constructor?
> > To avoid inconsistency. Here the parameters match the fields and it seemed to me that it would be better to update the order. 
> OK. But why not do the same thing for ObjCCategoriesVisitor?
The thing is that while the order of initializers needs to match the order of fields (and compilers catch it by generating a warning (and it's awesome), the order of ctor arguments doesn't need to match the order of fields (unless we are speaking about some readability concerns etc). The tool clang-reorder-fields handles correctly aggregate types & brace initializations (updates all the call sites) but doesn't change the order of ctor arguments (for now (v0)) - sometimes (for example when some fields use several arguments, or there is a non-trivial ctor body or there are other complications) it's not always easy to reason which order of ctor arguments is "natural" - and i try to preserve the old order (especially remembering about the compatible types and some call sites like emplace_back). 
Having said that - yea, the case of ObjCCategoriesVisitor is simple and i will update that diff as well (thanks for pointing out).


Repository:
  rL LLVM

https://reviews.llvm.org/D24754