On Feb 5, 2014, at 2:29 PM, Richard Smith <[email protected]> wrote:
> On Wed, Feb 5, 2014 at 12:43 PM, Ben Langmuir <[email protected]> wrote:
>
> On Feb 5, 2014, at 11:18 AM, Richard Smith <[email protected]> wrote:
>
> > I'm not especially fond of treating annot_module_begin and
> > annot_module_include differently. That'll mean we get completely different
> > diagnostics for a '#include' in a bad place depending on whether it's
> > including something in the same top-level module or something in a
> > different top-level module.
> >
>
> I approached this from the other side and didn't like that
> annot_module_include is handled so differently from @import.
>
> I think they're fundamentally different, at least currently:
>
> #include sometimes triggers a submodule build (in the same preprocessor +
> parser + sema) so must only appear at the top level in such a case. We also
> want the #include to behave (effectively) the same whether it textually
> includes a file or imports a module, so that people don't accidentally write
> code that means different things with compilers that support modules and with
> compilers that do not (or depending on which module a particular header is
> part of, or the order in which submodules of a module are built).
>
> Conversely, @import merely makes declaration and macro names visible. It
> never triggers an in-situ submodule build (sometimes a different module will
> be built in a separate thread and context, but that's not a problem). It
> can't (reliably) be used to import a submodule of the same module. So,
> there's no *technical* reason not to allow @import anywhere, as far as I can
> tell. That said, some of these properties are probably bugs in @import, and
> maybe it should act more like #include.
Sounds like you know much more about this than I do :)
When you say “sometimes a different module will be build in a separate thread
and context” does that include building a submodule of the current module, or
can that never happen?’
I agree that there is no technical reason @import can’t live anywhere, and as
you say I would like to constrain it to be more like #include in this respect.
>
> > I'm also not sure that the diagnostic change here is an improvement.
> > Previously:
> >
> > #include "not_module.h"
> > #include "module.h"
> >
> > ... where not_module.h contains:
> >
> > class X {
> > // ...
> > // whoops, forgot the '};'
> >
> > would give a "missing '}'" diagnostic, pointing at the '{' that is left
> > un-closed. It now gives a mysterious 'must be at the top level' diagnostic,
> > not pointing at the '}' any more, followed by more errors (because
> > 'module.h' didn't get imported and because the code after the #include is
> > still within the class). The error here was almost certainly that a '}' was
> > missing, so this seems like a regression.
>
> Ick, I’ll take a look at this case.
>
> >
> > In what cases does this change allow us to recover from errors better?
>
>
> Whenever an implicit import is not at top level, we currently give errors
> like ‘expected expression’, or ‘expected }’, pointing at the location on
> #include/#import, and give no hint as to how a preprocessor directive
> (#include) could be the source of the error.
>
> We can, and almost certainly should, customize the ExpectAndConsume
> diagnostic for the case where the current token is one of the annot_module_*
> tokens. I think that's probably the best way to attack this issue.
I’ll look into this.
>
> To make matters worse, once you break out of whatever parsing context you
> were in due to the module_include, any following code is likely to become
> errors.
>
> int foo() {
> int x = 5;
> #import “Bar.h” // expected }
> if (x == 6) {// expected identifier or (
> ++x; // expected identifier or (
> }
> return 1; // expected identifier of (
> } // extraneous closing brace
>
> Do you think this is actually a likely error case? I find it implausible that
> someone would deliberately include a modular header into the middle of some
> scope. I think it's vastly more likely that they missed a close brace before
> the include.
This was prompted by code like the following being spotted "in the wild" (where
the actual header is in a system module):
@implementation SomeClass
#import <system/header.h>
// … more stuff here
@end
I have no data to support it being a common case, but I’d like to improve the
error if possible.
>
> If this really is a likely scenario, then I think we should make the
> diagnostic for an unexpected annot_module_* a FatalError, because there's
> simply no way we can reasonably recover (we have two recovery options, and
> either of them is disastrous if we're in the other situation).
Wouldn’t this make your example even worse? Instead of getting a bogus error
(an #import not being at top level) followed by the actual error (missing })
you would only see the bogus one?
Ben_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
