<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jul 9, 2015 at 4:06 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>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></span><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></div></div></blockquote><div><br></div><div>No (I assume you meant the check in the other direction). Actually even my patch fails in some cases. There are other ways that things go wrong, at least the way we mark the top-level module as unavailable when we see a missing header. E.g.</div><div><br></div><div><p style="margin:0px;font-size:12px;font-family:Menlo">module nonrequired_missing_header {</p>
<p style="margin:0px;font-size:12px;font-family:Menlo"> module unsatisfied_requires {</p>
<p style="margin:0px;font-size:12px;font-family:Menlo"> header "nonrequired_missing_header/missing.h"</p>
<p style="margin:0px;font-size:12px;font-family:Menlo">+ module foo {</p>
<p style="margin:0px;font-size:12px;font-family:Menlo">+ header "nonrequired_missing_header/foomissing.h"</p>
<p style="margin:0px;font-size:12px;font-family:Menlo">+ }</p>
<p style="margin:0px;font-size:12px;font-family:Menlo"> requires nonexistent_feature</p>
<p style="margin:0px;font-size:12px;font-family:Menlo"> }</p>
<p style="margin:0px;font-size:12px;font-family:Menlo"> module fine_to_import {</p>
<p style="margin:0px;font-size:12px;font-family:Menlo"> header "nonrequired_missing_header/not_missing.h"</p>
<p style="margin:0px;font-size:12px;font-family:Menlo"> }</p>
<p style="margin:0px;font-size:12px;font-family:Menlo">}</p></div><div><br></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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><div><br></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"><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></span><div>Am I right in thinking that your patch causes regressions on valid input without a fix to this issue?</div></div></div></div></blockquote><div><br></div><div>I don't think so. At worst the current patch will trade a silent textual inclusion for a spurious missing header error. Since our previous behavior was not correct, it cannot be a regression. Anecdotally, a silent textual inclusion can be insanely hard to debug and so a spurious error with an easy-to-understand workaround is an improvement, if anything.</div><div> </div><div>-- Sean Silva</div><div><br></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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div>-- Sean Silva</div></font></span><span><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=WOASjoECLpg66arXkOB2wZrsA5RfsCyziV432QxQbuk&s=drVeWgnXrkeOvpfHY2LzcVcA6lJ4N6ySts-tODmRAz8&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10423</a><br>
<br>
<br>
<br>
</blockquote></span></div><br></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>