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. > 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. > 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. 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).
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
