[PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive

[Sean Silva via cfe-commits]

silvas added a subscriber: silvas.

================
Comment at: lib/Parse/Parser.cpp:2003
@@ +2002,3 @@
+    Diag(Tok, diag::err_unexpected_module_start);
+    // Recover by skipping content of the included submodule.
+    unsigned ModuleNesting = 1;
----------------
rsmith wrote:
> This is liable to produce bad follow-on diagnostics; I don't think this is a reasonable way to recover. I can see a few feasible options here:
> 
> 1) Emit a fatal error when this happens (suppressing further diagnostics)
> 2) Break out to the top level of parsing and resume from there (that is, assume that a modular header expects to be included at the top level and that the user didn't screw up their module map)
> 3) Enter the module and carry on parsing from here
> 
> My preference would be either (1) or (2). But either way, we should diagnose the still-open bracketing constructs, because the problem is likely to be a missing close brace at the end of an unrelated header file.
I strongly prefer (1). In all cases I have recorded in my notes when modularizing, the `error: expected '}'` diagnostic indicated one of two things:
- that a header needed to be marked textual in the module map.
- that a #include had to be moved to the top of the file (this case was likely behaving unexpectedly in the non-modules case and "happened to work").
For the sake of our users, it is probably best to immediately fatal error and suggest an alternative (the suggestions can be a separate patch; my recommendations are below).

I believe a user is most likely to run into this diagnostic when developing an initial set of module maps, so our diagnostic should be tailored to that audience. These users may have little experience with modules when they encounter this diagnostic, so a notes suggesting a likely remedy helps them develop confidence by providing authoritative advice (and avoiding a round trip to the documentation).

My fine-grained recommendations from experience are:
- #include inside extern "C": always meant to be modular, always the fix is to move it out of the braced block.
- #include inside namespace: always meant to be modular, always the fix is to move it out of the braced block.
- #include inside class: always textual (e.g. bringing in methods like TableGen .inc files; sometimes similar ".inc" files are manually written and not autogenerated. I have observed e.g. "classfoo_methods.h" files; another instance of this is stamping out ".def" files)
- #include inside array initializer: always textual. Typically ".def" files
- #include inside function: always textual. Typically ".def" files (e.g. generate a switch)

".inl" files are theoretically a problem inside namespaces, but I have not observed this issue in practice. In my experience code taking the effort to have ".inl" files is quite clean and avoids such textual dependencies. The issues I have seen in practice with ".inl" files usually end up coming out through a different part of clang. Improving that is for a separate patch though.


http://reviews.llvm.org/D11844