On Tue, Jun 16, 2015 at 7:21 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
> On Tue, Jun 16, 2015 at 7:10 PM, Sean Silva <chisophu...@gmail.com> wrote: > >> On Tue, Jun 16, 2015 at 2:38 PM, Richard Smith <rich...@metafoo.co.uk> >> wrote: >> >>> On Fri, Jun 12, 2015 at 8:29 PM, Sean Silva <chisophu...@gmail.com> >>> wrote: >>> >>>> ================ >>>> Comment at: lib/Lex/ModuleMap.cpp:346-347 >>>> @@ -345,4 +345,1 @@ >>>> - // Cannot use a module if it is unavailable. >>>> - if (!H.getModule()->isAvailable()) >>>> - continue; >>>> if (!Result || isBetterKnownHeader(H, Result)) >>>> ---------------- >>>> rsmith wrote: >>>> > Should we also teach `isBetterKnownHeader` to consider an available >>>> module to be better than an unavailable one? (If I have two modules >>>> covering a header file, and one of them requires "blah" and the other >>>> requires "!blah", we should presumably pick the available module.) >>>> I'm having a hard time thinking of a legitimate use for that, or in >>>> general to ever have two modules referencing the same file. Do you have >>>> something in particular in mind? >>>> >>>> But that does raise the larger question is how to make this insensitive >>>> to whatever order we are iterating here (which I assume is determined in >>>> some relatively fragile way by the order of command line arguments or order >>>> in the module map file). >>> >>> >>> That's a good question. We used to try to prefer the current module over >>> all others, and to prefer modules the current module uses over others, but >>> either that became broken at some point or never worked, and you removed >>> the last vestige of it in r239453. We should bring that back and make it >>> work... >>> >>> ... but it's an incomplete solution. One of the options for the >>> remaining cases would be to import *all* the tied-for-best modules for a >>> particular header if it's ambiguous, perhaps with a warning. >>> >> >> That sounds reasonable. Do you still want me to put the band-aid in >> isBetterKnownHeader? Or do you see that as another rule to use to try to >> have things "Just Work (TM)" for users? >> > > I'd like the "prefer the current module to anything else; prefer things we > can use over things we can't" rules added. I don't think they're a band-aid > really; if a library provides two interfaces with different sets of > headers, and another module explicitly depends on one of those, we > shouldn't warn about ambiguities with the other one, or diagnose undeclared > uses of the other one. > So concretely for the present patch just the change to isBetterKnownHeader to take into account of isAvailable? > > >> From a user's perspective, it is extremely jarring to have the compiler's >>>> behavior depend in a fragile way on such things (like the unix linker >>>> order-dependence fiasco). I would prefer to just have a hard error if two >>>> modules reference the same header, thus we wouldn't even be iterating here >>>> at all, and users will always be immediately alerted to errors. >>> >>> >>> This is a feature that we explicitly support, and some users (ourselves >>> included) rely on. Sometimes you have two logical libraries provided by the >>> same source code that have an overlapping set of headers. It would be more >>> philosophically clean to require such shared headers to be factored out >>> into a separate sub-library that the overlapping libraries can both depend >>> on, but an explicit goal of Clang's module support is to pragmatically >>> support existing code and configurations. >>> >> >> Can you give a more concrete example? I'm still having a hard time >> imagining this use case. >> > > Suppose you have a library that has a "light" version and a > "full-featured" version (where most, but perhaps not all, of the "light" > headers are part of the "full-featured" library), and you choose to > represent that as two modules (with overlapping headers) rather than three. > Okay, I can see that. -- Sean Silva
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits