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

[Richard Smith]

On Thu, Jul 9, 2015 at 3:47 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> On Wed, Jul 8, 2015 at 12:58 PM, Ben Langmuir <blangmuir at apple.com> wrote:
>
>> benlangmuir added a comment.
>>
>> > - Added better source location.
>>
>>
>> It should really be the requirement location, but I guess we don't store
>> that anywhere.  This can be improved separately.
>>
>> > - Updated testing. I renamed to auto-import-unavailable.cpp and
>> expanded testing as you asked for. Before, I was piggybacking on
>> missing-header.m and related files, but since that is a different code path
>> (and has different requirements and diagnostics), I thought it was better
>> to keep the test separate and self-contained.
>>
>> > - also emit note_implicit_top_level_module_import_here on missing
>> requirement.
>>
>>
>> Changes look good, thanks.
>>
>> I replied to your email about the issue when a "requires" clause comes
>> *after* a header declaration. I didn't actually hit that problem in the
>> wild - only in a synthetic test -  so maybe it can be addressed
>> separately.  If you and Richard agree, then from my perspective your patch
>> LGTM and we just need to get the Darwin hacks in before it lands.
>>
>
> This seems orthogonal. It is due to Module::markUnavailable having
> multiple bailouts that are incorrect if MissingRequirement would have been
> changed on any of the submodules during the traversal. The attached patch
> fixes that, but I think opens us up to O(n^2) behavior.
>

Can we fix that by changing the bailout condition to:

  if (MissingRequirement ? !IsAvailable : IsMissingRequirement)

?

Fixing this (probably not with my simple patch attached here) seems like a
> good change, but unrelated to this patch.
>

Am I right in thinking that your patch causes regressions on valid input
without a fix to this issue?


> -- Sean Silva
>
>
>>
>> > Ben: hopefully the present patch is complete enough for you to start
>>
>> >  figuring out exactly what hacks to add and where.
>>
>>
>> Definitely.  I can work from this, thanks.
>>
>>
>> http://reviews.llvm.org/D10423
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150709/8c1f15fc/attachment.html>