<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jul 9, 2015 at 3:47 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"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Wed, Jul 8, 2015 at 12:58 PM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">benlangmuir added a comment.<br>
<span><br>
> - Added better source location.<br>
<br>
<br>
</span>It should really be the requirement location, but I guess we don't store that anywhere.  This can be improved separately.<br>
<span><br>
> - Updated testing. I renamed to auto-import-unavailable.cpp and expanded testing as you asked for. Before, I was piggybacking on missing-header.m and related files, but since that is a different code path (and has different requirements and diagnostics), I thought it was better to keep the test separate and self-contained.<br>
<br>
> - also emit note_implicit_top_level_module_import_here on missing requirement.<br>
<br>
<br>
</span>Changes look good, thanks.<br>
<br>
I replied to your email about the issue when a "requires" clause comes *after* a header declaration. I didn't actually hit that problem in the wild - only in a synthetic test -  so maybe it can be addressed separately.  If you and Richard agree, then from my perspective your patch LGTM and we just need to get the Darwin hacks in before it lands.<br></blockquote><div><br></div></span><div>This seems orthogonal. It is due to Module::markUnavailable having multiple bailouts that are incorrect if MissingRequirement would have been changed on any of the submodules during the traversal. The attached patch fixes that, but I think opens us up to O(n^2) behavior.</div></div></div></div></blockquote><div><br></div><div>Can we fix that by changing the bailout condition to:</div><div><br></div><div>  if (MissingRequirement ? !IsAvailable : IsMissingRequirement)</div><div> <br></div><div>?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Fixing this (probably not with my simple patch attached here) seems like a good change, but unrelated to this patch.</div></div></div></div></blockquote><div><br></div><div>Am I right in thinking that your patch causes regressions on valid input without a fix to this issue?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="HOEnZb"><font color="#888888"><div>-- Sean Silva</div></font></span><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><br>
> Ben: hopefully the present patch is complete enough for you to start<br>
<br>
>  figuring out exactly what hacks to add and where.<br>
<br>
<br>
</span>Definitely.  I can work from this, thanks.<br>
<br>
<br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10423&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=gDn2hlH35Yf-KDAe-6SvZBrvsBv7VK3UrrJciTWznOs&s=wIP4cY4_2OB0FDLTo0VSBqN7EjnYX46Hw4WE9DTD-W0&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10423</a><br>
<br>
<br>
<br>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>