<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jul 2, 2015, at 5:02 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com" class="">chisophugis@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Wed, Jul 1, 2015 at 3:32 PM, Ben Langmuir <span dir="ltr" class=""><<a href="mailto:blangmuir@apple.com" target="_blank" class="">blangmuir@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I tried this patch out and found a few issues I've commented on inline. It also exposes the two issues below:<br class="">
<br class="">
1. This patch exposes a compatibility issue with our 'Darwin' module map file (the module map that covers most of /usr/include on Darwin platforms).<br class="">
<br class="">
error: module 'Darwin.C.excluded' requires feature 'excluded'<br class="">
<br class="">
This "excluded" submodule was used as a hack for "assert.h" before the explicit support for "exclude" and "textual" headers were added to clang. After this patch, it won't be possible to include assert.h on Darwin, because the module it is in is missing a requirement. I think it would be very bad to break compatibility here, so I propose we add a compatibility hack to module map parsing that treats a module with a the requirement "excluded" as if all its headers were declared with "exclude". What do you think? I'm happy to implement this.<br class="">
<br class="">
2. If a require decl is written *after* a header decl in a module, that header still gets treated as required. We should fix that before this is committed. E.g.<br class="">
<br class="">
<br class="">
<br class="">
module Top {<br class="">
module A { header "foo.h" requires nope }<br class="">
module B { requires nope header "bar.h" }<br class="">
}<br class="">
<br class="">
We'll complain about missing foo.h, but not about bar.h.<br class=""></blockquote><div class=""><br class=""></div><div class="">I can't reproduce this locally (with or without my patch). What concrete command line are you using, and what are you seeing?</div></div></div></div></div></blockquote><div><br class=""></div><div>You can see this by modifying your test case <font face="Menlo" class="">``</font><span style="font-family: Menlo;" class="">nonrequired_missing_header``:</span></div><div><br class=""></div><div><div><font face="Menlo" class=""> module nonrequired_missing_header {</font></div><div><font face="Menlo" class=""> module unsatisfied_requires {</font></div><div><font face="Menlo" class=""> requires nonexistent_feature</font></div><div><font face="Menlo" class=""> header "nonrequired_missing_header/missing.h”</font></div><div><font face="Menlo" class=""> }</font></div><div><div><font face="Menlo" class="">+ module unsatisfied_requires_after {</font></div><div><font face="Menlo" class="">+ header "nonrequired_missing_header/missing2.h”</font></div><div><font face="Menlo" class="">+ requires nonexistent_feature</font></div><div><font face="Menlo" class="">+ }</font></div></div><div><font face="Menlo" class=""> module fine_to_import {</font></div><div><font face="Menlo" class=""> header "nonrequired_missing_header/not_missing.h"</font></div><div><font face="Menlo" class=""> }</font></div><div><font face="Menlo" class=""> }</font></div><div><br class=""></div><div><br class=""></div><div>error: header 'nonrequired_missing_header/missing2.h' not found</div><div><br class=""></div><div>Ben</div></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><div class="">-- Sean Silva</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="">
<br class="">
================<br class="">
Comment at: lib/Lex/PPDirectives.cpp:1671<br class="">
@@ +1670,3 @@<br class="">
+ Diag(MissingHeader.FileNameLoc, diag::err_module_unavailable)<br class="">
+ << M->getFullModuleName() << Requirement.second << Requirement.first;<br class="">
+ }<br class="">
----------------<br class="">
1. This uses MissingHeader.FileNameLoc, which we just proved is invalid :-)<br class="">
2. We should also show diag::note_implicit_top_level_module_import_here in this diagnostic.<br class="">
<br class="">
================<br class="">
Comment at: test/Modules/missing-header.cpp:13<br class="">
@@ +12,2 @@<br class="">
+// We should not attempt to build the module.<br class="">
+// CHECK-NOT: could not build module 'missing_headers'<br class="">
----------------<br class="">
We should have a test for including a header from a module with a missing requirement. And one where a missing header from a different submodule is okay because the submodule is missing a requirement.<br class="">
<div class="HOEnZb"><div class="h5"><br class="">
<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=JGu7BPOjqJDv9iloJXZOWGyy3q2Ksbyuhs4GbtOJXVg&s=yLUPEE0J6Mi6Jcs_XvSeJksS7AapPdc2Wc3th_1sCqU&e=" rel="noreferrer" target="_blank" class="">http://reviews.llvm.org/D10423</a><br class="">
<br class="">
EMAIL PREFERENCES<br class="">
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=JGu7BPOjqJDv9iloJXZOWGyy3q2Ksbyuhs4GbtOJXVg&s=5o0kVID-tPnnMrEWgYZUQPlyv3kh6YdlfkUn1svjbzo&e=" rel="noreferrer" target="_blank" class="">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br class="">
<br class="">
<br class="">
</div></div></blockquote></div><br class=""></div></div>
</div></blockquote></div><br class=""></body></html>