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

Александр Шапошников via cfe-commits cfe-commits at lists.llvm.org
Thu Sep 22 13:36:02 PDT 2016


>What's the purpose of this change
PaddingChecker generates a report on it,
yeah, you are right that it won't save too much (just a little), but
it was easy to fix. Another reason why i am doing this -
it reduces the number of reports produced by static analyzer (less noisy)
- 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)).

On Thu, Sep 22, 2016 at 1:04 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> 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.
>
> On 22 Sep 2016 12:41 pm, "Alexander Shaposhnikov" <shal1t712 at gmail.com>
> wrote:
>
>> 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
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160922/3e9ba41c/attachment.html>


More information about the cfe-commits mailing list