> On Jul 2, 2015, at 5:02 PM, Sean Silva <chisophu...@gmail.com> wrote:
>
>
>
> On Wed, Jul 1, 2015 at 3:32 PM, Ben Langmuir <blangm...@apple.com
> <mailto:blangm...@apple.com>> wrote:
> I tried this patch out and found a few issues I've commented on inline. It
> also exposes the two issues below:
>
> 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).
>
> error: module 'Darwin.C.excluded' requires feature 'excluded'
>
> 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.
>
> 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.
>
>
>
> module Top {
> module A { header "foo.h" requires nope }
> module B { requires nope header "bar.h" }
> }
>
> We'll complain about missing foo.h, but not about bar.h.
>
> I can't reproduce this locally (with or without my patch). What concrete
> command line are you using, and what are you seeing?
You can see this by modifying your test case ``nonrequired_missing_header``:
module nonrequired_missing_header {
module unsatisfied_requires {
requires nonexistent_feature
header "nonrequired_missing_header/missing.h”
}
+ module unsatisfied_requires_after {
+ header "nonrequired_missing_header/missing2.h”
+ requires nonexistent_feature
+ }
module fine_to_import {
header "nonrequired_missing_header/not_missing.h"
}
}
error: header 'nonrequired_missing_header/missing2.h' not found
Ben
>
> -- Sean Silva
>
>
>
> ================
> Comment at: lib/Lex/PPDirectives.cpp:1671
> @@ +1670,3 @@
> + Diag(MissingHeader.FileNameLoc, diag::err_module_unavailable)
> + << M->getFullModuleName() << Requirement.second <<
> Requirement.first;
> + }
> ----------------
> 1. This uses MissingHeader.FileNameLoc, which we just proved is invalid :-)
> 2. We should also show diag::note_implicit_top_level_module_import_here in
> this diagnostic.
>
> ================
> Comment at: test/Modules/missing-header.cpp:13
> @@ +12,2 @@
> +// We should not attempt to build the module.
> +// CHECK-NOT: could not build module 'missing_headers'
> ----------------
> 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.
>
> http://reviews.llvm.org/D10423 <http://reviews.llvm.org/D10423>
>
> EMAIL PREFERENCES
> http://reviews.llvm.org/settings/panel/emailpreferences/
> <http://reviews.llvm.org/settings/panel/emailpreferences/>
>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
