<div dir="ltr"><span style="font-size:12.8px">>What's the purpose of this change</span><div><span style="font-size:12.8px">PaddingChecker generates a report on it, </span></div><div><span style="font-size:12.8px">yeah, you are right that it won't save too much (just a little), but</span></div><div><span style="font-size:12.8px">it was easy to fix. Another reason why i am doing this - </span></div><div><span style="font-size:12.8px">it reduces the number of reports produced by static analyzer (less noisy) </span></div><div><span style="font-size:12.8px">- and since it saves smth (and i don't see other regressions) - decided to change it. (btw - the order FirstId, IsKeyDecl, MergeWith is another option and also looks natural (if i am not mistaken)).</span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 22, 2016 at 1:04 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">What's the purpose of this change? We only put these objects in the stack, and never have many of them at once. If we don't care about size, a natural field order seems preferable to one that minimises padding.<div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On 22 Sep 2016 12:41 pm, "Alexander Shaposhnikov" <<a href="mailto:shal1t712@gmail.com" target="_blank">shal1t712@gmail.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">alexshap added inline comments.<br>
<br>
================<br>
Comment at: lib/Serialization/ASTReaderDec<wbr>l.cpp:154<br>
@@ -153,3 +153,3 @@<br>
     public:<br>
-      RedeclarableResult(GlobalDeclI<wbr>D FirstID, Decl *MergeWith, bool IsKeyDecl)<br>
-          : FirstID(FirstID), MergeWith(MergeWith), IsKeyDecl(IsKeyDecl) {}<br>
+      RedeclarableResult(Decl *MergeWith, GlobalDeclID FirstID, bool IsKeyDecl)<br>
+          : MergeWith(MergeWith), FirstID(FirstID), IsKeyDecl(IsKeyDecl) {}<br>
----------------<br>
vsk wrote:<br>
> alexshap wrote:<br>
> > vsk wrote:<br>
> > > Why do you need to change the order of the parameters in the constructor?<br>
> > To avoid inconsistency. Here the parameters match the fields and it seemed to me that it would be better to update the order.<br>
> OK. But why not do the same thing for ObjCCategoriesVisitor?<br>
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).<br>
Having said that - yea, the case of ObjCCategoriesVisitor is simple and i will update that diff as well (thanks for pointing out).<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D24754" rel="noreferrer" target="_blank">https://reviews.llvm.org/D2475<wbr>4</a><br>
<br>
<br>
<br>
</blockquote></div></div>
</div></div></blockquote></div><br></div>