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

[Sean Silva]

On Wed, Jul 8, 2015 at 2:05 PM, Richard Smith <richard at 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.`.


>  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.


>  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.

-- 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150710/fd1b65fe/attachment.html>