On Wed, Jun 26, 2013 at 2:54 PM, Reid Kleckner <[email protected]> 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 <[email protected]>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 <[email protected]> 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 >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
