<div dir="ltr">Sean, could you review the test cases in <a href="http://reviews.llvm.org/D11438">http://reviews.llvm.org/D11438</a> too?<div><br><div>New features (or bug fixes) introduced in this patch really should only be executed when x86_64 f128 type is configured to stay in SSE registers.</div><div>We don't turn on that configuration in this partial patch, so all unit tests in D11438 won't apply. Is there some new test we can add to test other types for this patch?</div><div><br></div><div>After my initial submit of D11438 in Jul 22 2015, I have received virtually no comment on those unit tests yet. I do feel the risk of test coverage for both new x86_64 f128 config and existing configurations.</div><div><br></div><div>I explained before in patch D11438 comments that a partial change of x86_64 f128 is worse than no change at all, as we would rather have a working non-gcc compatible x86_64 long double type than a buggy type or crashing compiler. So a partial commit like this has only benefit to verify partial regression to non x86_64 f128 types, which has lower possibility and I have no trouble to revert them all at once.</div><div><br></div><div>The main battle will be for Android platform to verify the correctness of x86_64 f128 type after the configuration is submitted in the follow up patch.</div><div><br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 2, 2015 at 6:21 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">silvas added a comment.<br>
<span class=""><br>
In <a href="http://reviews.llvm.org/D15134#300978" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15134#300978</a>, @chh wrote:<br>
<br>
> It would be easier to keep track of these changes together, right?<br>
<br>
<br>
</span>No, it is actually the opposite. The more fine-grained the individual commits, the easier it is for a reviewer to review, and the easier it is to identify what caused an issue if an issue does show up. Tests are also easier to write for fine-grained commits (or, it is easier to identify that a fine-grained commit does not change externally visible behavior and so doesn't require a test).<br>
<br>
Also, the changes you are proposing here seem to fix bugs / introduce new functionality. Therefore tests are required.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D15134" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15134</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>