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

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1018-1019
@@ -1017,2 +1017,4 @@
   "expected ';' after module name">;
+def err_unexpected_module_end : Error<"unexpected module end">;
+def err_unexpected_module_start : Error<"submodule cannot be started here">;
 }
----------------
These diagnostics are phrased from the point of view of the implementation, and should instead be phrased from the point of view of the user.

The first diagnostic would be more useful if it said something like "missing '}' at end of module". That's pretty close to the diagnostic we already produce; maybe instead of changing the behavior of this case you could improve `BalancedDelimiterTracker` to use a custom diagnostic for the case where the missing closing delimiter is instead an end-of-module token?

The second diagnostic seems like an improvement, but from the user's perspective, this wasn't the start of a submodule, it was a module import / `#include`. Maybe just use `diagnoseMisplacedModuleImport` here?

================
Comment at: lib/Parse/ParseDeclCXX.cpp:3075
@@ +3074,3 @@
+                      tok::annot_module_end)) {
+        if (tryParseMisplacedModuleImport())
+          continue;
----------------
This shouldn't be named `try*` if it has a precondition that the current token is an EoM token. (The `try` form, if there is one, should check for that token itself.)

================
Comment at: lib/Parse/Parser.cpp:1999
@@ +1998,3 @@
+  case tok::annot_module_end:
+    Diag(Tok, diag::err_unexpected_module_end);
+    break;
----------------
This will result in you producing two errors "unexpected end of module" and "missing }" for each construct that is still open at the end of the module. I don't think that's what we want.

================
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;
----------------
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.

================
Comment at: lib/Parse/Parser.cpp:2018
@@ +2017,3 @@
+    // Module import found where it should not be, for instance, inside a
+    // namespace. Recover by ignoring the import.
+    Actions.diagnoseMisplacedModuleImport(reinterpret_cast<Module *>(
----------------
Again, this will recover very poorly. In this case, our best bet seems to be to recover by importing the module.


http://reviews.llvm.org/D11844