<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 10, 2015 at 1:48 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 2:05 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">rsmith added inline comments.<br>
<br>
================<br>
Comment at: lib/Lex/PPDirectives.cpp:1665<br>
@@ +1664,3 @@<br>
+ // unavailable, diagnose the situation and bail out.<br>
+ if (SuggestedModule && !SuggestedModule.getModule()->isAvailable()) {<br>
+ clang::Module::Requirement Requirement;<br>
----------------<br>
I think this whole block should be moved down to line 1761 or so:<br>
<br>
1. The code currently ensures that it always calls InclusionDirective on the callback object for every `#include`, even if that include fails.<br></blockquote><div><br></div></span><div>I don't think this is true. There is at least one return above it in the block `// We hit an error processing the import. Bail out.`.</div></div></div></div></blockquote><div><br></div><div>That looks like a bug.</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"><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">
2. We don't yet know that the file is actually part of `SuggestedModule`; it could be in the directory of an umbrella header whose module is unavailable, but it might not be part of that module.<br></blockquote><div><br></div></span><div>Sounds like a bug in the thing suggesting SuggestedModule? Or am I not understanding what the contract is for SuggestedModule? If I'm understanding what you're saying, it sounds like this is wholly not the correct place to check this.</div></div></div></div></blockquote><div><br></div><div>This is the nature of umbrella headers: we don't know which headers they cover until we build the corresponding module. SuggestedModule is just a suggestion until we've tried to load it and checked whether the header is actually covered by the module. (Yes, this is horrible, and I wish we didn't have these.)</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"><span class=""><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">
3. If we somehow got a suggested module but no file, we should not produce additional spurious diagnostics beyond the "file not found" diagnostic we already produced.<br></blockquote><div><br></div></span><div>Is that actually possible? Sounds like a broken invariant.</div></div></div></div></blockquote><div><br></div><div>Yes, I think you're right.</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">
<br>
================<br>
Comment at: test/Modules/Inputs/auto-import-unavailable/module.modulemap:9<br>
@@ +8,3 @@<br>
+ requires nonexistent_feature<br>
+ header "nonrequired_missing_header/missing.h"<br>
+ }<br>
----------------<br>
Please add some content to the empty files (even if just a comment) or this test will misbehave on content-addressed file systems, where all the empty files will be treated as the same file.<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=tx12LoYOst8vPjqn56Wz7HAg_vR0GGQvI1wzFCoYuFM&s=JOYPVLm3SnA50c5_ZEeupNTAz-QVk71-e8eOivltI50&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10423</a><br>
<br>
<br>
<br>
</blockquote></span></div><br></div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>