On Fri, Jul 10, 2015 at 1:48 PM, Sean Silva <chisophu...@gmail.com> wrote:
>
>
> On Wed, Jul 8, 2015 at 2:05 PM, Richard Smith <rich...@metafoo.co.uk>
> wrote:
>
>> rsmith added inline comments.
>>
>> ================
>> Comment at: lib/Lex/PPDirectives.cpp:1665
>> @@ +1664,3 @@
>> + // unavailable, diagnose the situation and bail out.
>> + if (SuggestedModule && !SuggestedModule.getModule()->isAvailable()) {
>> + clang::Module::Requirement Requirement;
>> ----------------
>> I think this whole block should be moved down to line 1761 or so:
>>
>> 1. The code currently ensures that it always calls InclusionDirective on
>> the callback object for every `#include`, even if that include fails.
>>
>
> I don't think this is true. There is at least one return above it in the
> block `// We hit an error processing the import. Bail out.`.
>
That looks like a bug.
2. We don't yet know that the file is actually part of `SuggestedModule`;
>> it could be in the directory of an umbrella header whose module is
>> unavailable, but it might not be part of that module.
>>
>
> Sounds like a bug in the thing suggesting SuggestedModule? Or am I not
> understanding what the contract is for SuggestedModule? If I'm
> understanding what you're saying, it sounds like this is wholly not the
> correct place to check this.
>
This is the nature of umbrella headers: we don't know which headers they
cover until we build the corresponding module. SuggestedModule is just a
suggestion until we've tried to load it and checked whether the header is
actually covered by the module. (Yes, this is horrible, and I wish we
didn't have these.)
3. If we somehow got a suggested module but no file, we should not produce
>> additional spurious diagnostics beyond the "file not found" diagnostic we
>> already produced.
>>
>
> Is that actually possible? Sounds like a broken invariant.
>
Yes, I think you're right.
> -- Sean Silva
>
>
>>
>> ================
>> Comment at: test/Modules/Inputs/auto-import-unavailable/module.modulemap:9
>> @@ +8,3 @@
>> + requires nonexistent_feature
>> + header "nonrequired_missing_header/missing.h"
>> + }
>> ----------------
>> Please add some content to the empty files (even if just a comment) or
>> this test will misbehave on content-addressed file systems, where all the
>> empty files will be treated as the same file.
>>
>>
>> http://reviews.llvm.org/D10423
>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10423&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=tx12LoYOst8vPjqn56Wz7HAg_vR0GGQvI1wzFCoYuFM&s=JOYPVLm3SnA50c5_ZEeupNTAz-QVk71-e8eOivltI50&e=>
>>
>>
>>
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
