[PATCH] [modules] PR20507: Avoid silent textual inclusion.

[Sean Silva]

On Tue, Jun 16, 2015 at 7:21 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> On Tue, Jun 16, 2015 at 7:10 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>> On Tue, Jun 16, 2015 at 2:38 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>>
>>> On Fri, Jun 12, 2015 at 8:29 PM, Sean Silva <chisophugis at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150616/1ad352ae/attachment.html>