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 <[email protected]> wrote: > On Wed, Jul 10, 2013 at 10:01 PM, Eli Friedman <[email protected]>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. >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
