<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jul 27, 2017 at 8:30 AM Dehao Chen via Phabricator via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">danielcdh marked an inline comment as done.<br>
danielcdh added a comment.<br>
<br>
Thanks for the review!<br>
<br>
In <a href="https://reviews.llvm.org/D35746#822498" rel="noreferrer" target="_blank">https://reviews.llvm.org/D35746#822498</a>, @chandlerc wrote:<br>
<br>
> LGTM with a tiny tweak below.<br>
><br>
> Would be good to add a test that this flag is being honored, either in this patch or in a follow-up.<br>
<br>
<br>
I'll add the test in a follow-up patch. Could you help suggest how to add a test for this? Shall I just invoke "clang -cc1 -fexperimental-new-pass-manager" and check if discriminator is generated? That seems an integration test.<br></blockquote><div><br>Yeah, things that interact between Clang and LLVM that aren't part of IR basically slip through the cracks of the nice separation of testing here. Either they go untested, or are tested in a more 'integration' like test - though trying to find the minimal thing to examine that's not testing all the features, etc.<br><br>If there's a clang argument that dumps the pass sequence? Or some other 'earlier' observable property like that? (how's the -fexperimental-new-pass-manager flag tested, for example? maybe it isn't)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D35746" rel="noreferrer" target="_blank">https://reviews.llvm.org/D35746</a><br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>