On Wed, Jul 1, 2015 at 3:32 PM, Ben Langmuir <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? -- 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 > > EMAIL PREFERENCES > 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