On Wed, Feb 5, 2014 at 3:00 PM, Ben Langmuir <[email protected]> wrote:
> > 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?' > That doesn't happen today. If you have a.h and b.h in module X, and we happen to decide to build a.h before b.h, and a.h contains: @import X.b; ... then that @import is a no-op. If it had been #include "b.h" we would have built the submodule X.b immediately. See test/Modules/Inputs/submodules/import-self-b.h for a complete example. I suspect this is a bug =) 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. > OK, I can certainly believe that people do this somewhat deliberately inside an @implementation (but not so much in a class or function). We occasionally see people try to put #includes inside extern "C" and namespaces in C++ (mistakenly believing they're including a C header, usually), and that's another case where assuming that we should be at the top level might not work out very well. Conversely, I have data to support the position that missing-}-at-end-of-file is a big problem in C++, and our diagnostics for it are currently terrible. For a missing } in a modular header, we have a great story, and we *know* the 'expected }' diagnostic is correct, but for a non-modular header, the best answer we have today is to hope that the *next* include will be of a modular header. So I think recovering by skipping the include and staying in the nested scope is also not going to work out well. 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? > I was imagining that we'd only produce the one error (the error that is currently "expected '}'", but with a change to ExpectAndConsume would presumably become something more like "fatal error: module import not at top level; expected '}'" with a note pointing to the '{' as normal).
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
