[PATCH] Microsoft C Record Layout

[Eli Friedman]

On Wed, Jun 26, 2013 at 2:54 PM, Reid Kleckner <rnk at google.com> wrote:

> The motivation is that C++ record layout for the Microsoft C++ ABI is
> sufficiently different to warrant its own builder.  There's already lots of
> code in there for virtual bases and vtordisps and other fun things that
> don't concern Itanium.
>
> Ultimately, we should be able to do MS C struct bitfield layout in this
> builder in addition to the C++ record layout.
>
> This patch doesn't actually do anything with C++ record layout yet to keep
> the diff smaller and be more incremental.
>
> Does that make sense to you?
>

If you think the current MS C++ ABI support in RecordLayoutBuilder.cpp is
unmaintainable, then yes, it makes sense to do something to separate it
out.  That said, I still don't understand why duplicating the field layout
routines helps us in any way; the current IsMsStruct checks are much more
concise than duplicating the routines.  If you want to keep the MS and
Itanium C++ code in separate files, we can introduce a common base class or
something like that.

-Eli



>
>
> On Wed, Jun 26, 2013 at 5:41 PM, Eli Friedman <eli.friedman at gmail.com>wrote:
>
>> Sorry, I wasn't really paying attention to this patch before.
>>
>> I've been looking through the review comments and I can't seem to find
>> the motivation for why you're writing a new layout builder from scratch
>> (and duplicating a bunch of code) instead of taking advantage of the
>> existing IsMsStruct flag in RecordLayoutBuilder.cpp.  Is the difference
>> between IsMsStruct and what you're implementing really that substantial?
>>
>> I saw earlier in the review comments you were alluding to CodeGen
>> crashes; that shouldn't be an issue as of r185018.
>>
>> Please put the SemaDecl.cpp change into a separate patch.
>>
>> -Eli
>>
>>
>> On Wed, Jun 26, 2013 at 2:23 PM, Warren Hunt <whunt at google.com> wrote:
>>
>>>    Addressing 2nd round of feedback, enabling a Sema error, clang format
>>> and some minor fixes.
>>>
>>> Hi rnk, rsmith,
>>>
>>> http://llvm-reviews.chandlerc.com/D1026
>>>
>>> CHANGE SINCE LAST DIFF
>>>   http://llvm-reviews.chandlerc.com/D1026?vs=2536&id=2578#toc
>>>
>>> Files:
>>>   include/clang/AST/ASTContext.h
>>>   include/clang/Sema/Sema.h
>>>   lib/AST/CMakeLists.txt
>>>   lib/AST/MicrosoftRecordLayoutBuilder.cpp
>>>   lib/AST/RecordLayoutBuilder.cpp
>>>   lib/Sema/SemaDecl.cpp
>>>   test/Sema/ms_bitfield_layout.c
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130626/f7f06e07/attachment.html>