<div dir="ltr">On Wed, Jul 10, 2013 at 10:01 PM, Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Please split out the SemaDecl.cpp changes into another patch.<br>
<br>
Some of the changes to test/Sema/ms_class_layout.cpp don't look right;<br>
are you sure they're accurate?<br>
<div class="im"><br>
>   * 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.<br>

<br>
</div>It's also worth noting that you didn't implement -Wpadded...<br></blockquote><div><br></div><div style>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.</div>
</div></div></div>