[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] D10423: [modules] PR20507: Avoid silent textual inclusion.
From: Richard Smith <richard () metafoo ! co ! uk>
Date: 2015-07-13 21:02:10
Message-ID: CAOfiQqn1ZvauAmk+MhRS7LQnOBLdT_NNNQ561J1EFGGmpgc0-w () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Fri, Jul 10, 2015 at 1:51 PM, Sean Silva <chisophugis@gmail.com> wrote:
>
>
> On Wed, Jul 8, 2015 at 7:16 PM, Richard Smith <richard@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;
>> ----------------
>> rsmith wrote:
>> > rsmith wrote:
>> > > 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.
>> > > 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.
>> > > 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.
>> > One other thing: we should only diagnose when `ShouldEnter` is `false`.
>> If we're entering the file anyway, we don't need the module to be
>> available; only those files within it that we're actually entering need to
>> be present.
>> Hmm, that's not quite right (`ShouldEnter` will be `false` the second
>> time we reach a header with an include guard too). I think the right thing
>> is: don't diagnose this if the suggested module's top-level module is
>> either the current module or the "implementation of" module (that is, if
>> it's a module we'll /always/ enter textually).
>>
>
> What are the semantics of the "current" module and the "implementation of"
> module? I.e. what do they mean in general?
>
They mean that the current file is notionally part of the specified module.
One consequence is that the headers for that module are textually included
rather than being imported. (And for some build systems, only a minimal set
of files necessary for the build are available at compile time, so we
cannot assume that the current module will always be available.)
As for why we have two of these, that seems to simply be a mistake. We
should instead have a single "current module" value and a flag to say
whether or not we're compiling the module interface.
[Attachment #5 (text/html)]
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 10, 2015 \
at 1:51 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" \
target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div \
class="gmail_quote"><span class="">On Wed, Jul 8, 2015 at 7:16 PM, Richard Smith \
<span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" \
target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div><div>rsmith added inline comments.<br> <br>
================<br>
Comment at: lib/Lex/PPDirectives.cpp:1665<br>
@@ +1664,3 @@<br>
+ // unavailable, diagnose the situation and bail out.<br>
+ if (SuggestedModule && !SuggestedModule.getModule()->isAvailable()) \
{<br> + clang::Module::Requirement Requirement;<br>
----------------<br>
rsmith wrote:<br>
> rsmith wrote:<br>
> > I think this whole block should be moved down to line 1761 or so:<br>
> ><br>
> > 1. The code currently ensures that it always calls InclusionDirective on \
the callback object for every `#include`, even if that include fails.<br> > > \
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.<br> > > 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.<br> > One other thing: \
we should only diagnose when `ShouldEnter` is `false`. If we're entering the file \
anyway, we don't need the module to be available; only those files within it that \
we're actually entering need to be present.<br> </div></div>Hmm, that's not \
quite right (`ShouldEnter` will be `false` the second time we reach a header with an \
include guard too). I think the right thing is: don't diagnose this if the \
suggested module's top-level module is either the current module or the \
"implementation of" module (that is, if it's a module we'll \
/always/ enter textually).<br></blockquote><div><br></div></span><div>What are the \
semantics of the "current" module and the "implementation of" \
module? I.e. what do they mean in \
general?</div></div></div></div></blockquote><div><br></div><div>They mean that the \
current file is notionally part of the specified module. One consequence is that the \
headers for that module are textually included rather than being imported. (And for \
some build systems, only a minimal set of files necessary for the build are available \
at compile time, so we cannot assume that the current module will always be \
available.)</div><div><br></div><div>As for why we have two of these, that seems to \
simply be a mistake. We should instead have a single "current module" value \
and a flag to say whether or not we're compiling the module \
interface.</div></div></div></div>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic