[PATCH] Microsoft C++ Record Layout
whunt at google.com
Thu Jul 11 10:06:59 PDT 2013
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...
More information about the cfe-commits