[PATCH] Microsoft C++ Record Layout

[Warren Hunt]

Yes, I should have noted there are two warnings that I haven't implemented.
 It's much easier to drop them into the builder than it is to add them to
Sema, although it makes more logical sense to put them in Sema (which would
be a different patch).  I'm okay with either, are there any strong votes?


On Thu, Jul 11, 2013 at 6:22 AM, Reid Kleckner <rnk at google.com> wrote:

> On Wed, Jul 10, 2013 at 10:01 PM, Eli Friedman <eli.friedman at gmail.com>wrote:
>
>> Please split out the SemaDecl.cpp changes into another patch.
>>
>> Some of the changes to test/Sema/ms_class_layout.cpp don't look right;
>> are you sure they're accurate?
>>
>> >   * It's still in two files and doesn't use CRTP.  The net overlap
>> between the current record builder and this one is 7 member variables, 1
>> typedef and 10 functions with the same names but different functionality.
>>  The closest functionality is laying out fields (non-bit-fields) but it
>> rounds in a slightly different place and is only a few lines of code.
>>  Basically although in spirit the ms and non-ms builders achieve similar
>> goals, they do so in completely different ways.  I would use an analogy of
>> implementing a dictionary using a B-tree or a red-black tree.  They may
>> both implement a dictionary using a tree but don't actually have much of
>> any share-able code.
>>
>> It's also worth noting that you didn't implement -Wpadded...
>>
>
> I would argue that -Wpadded should be in Sema operating on the layouts
> produced by AST, but it looks like the field sizes aren't recorded in the
> layout and recomputing them is not trivial.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130711/b02a69f9/attachment.html>