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

[Richard Smith via cfe-commits]

rsmith added a subscriber: rsmith.
rsmith added a comment.

This doesn't recover from the errors properly any more (leaving junk behind in the 'current module' stack), and somewhat breaks the original purpose of these checks, which is to detect and diagnose missing close braces. Here's what I suggest:

1. Keep the `isEofOrEom` calls as-is.
2. Diagnose the unexpected EOM tokens as you do now (but customize the diagnostics in the case of a module_end token).
3. Once you've diagnosed an EOM token, don't consume it; instead update the annotation so you know not to diagnose it again, and bail out of parsing the current construct.

Then, when we hit an EOM, we'll diagnose it once at the innermost level, bail out all the way to the top level, and then handle it properly.


================
Comment at: include/clang/Sema/Sema.h:1781-1784
@@ -1780,1 +1780,6 @@
 
+  /// \brief Check if module import may be found in the specified context,
+  /// emit error if not.
+  void checkModuleImportContext(Module *M, SourceLocation ImportLoc,
+                                DeclContext *DC);
+
----------------
I don't think this is the right interface. The calls from the parser expect an error to be issued, and don't do the right thing if the module import is valid, so this should instead be called something more like `diagnoseMisplacedModuleImport`, and should either assert if the current context allows module imports or not perform the check.

================
Comment at: lib/Parse/ParseDeclCXX.cpp:2856-2863
@@ -2855,1 +2855,10 @@
 
+  if (Tok.isOneOf(tok::annot_module_include,
+                  tok::annot_module_begin,
+                  tok::annot_module_end)) {
+    Actions.checkModuleImportContext(reinterpret_cast<Module *>(
+        Tok.getAnnotationValue()), Tok.getLocation(), Actions.CurContext);
+    ConsumeToken();
+    return DeclGroupPtrTy();
+  }
+
----------------
Please factor this out into a `bool TryParseMisplacedModuleImport()` function.

================
Comment at: lib/Parse/ParseDeclCXX.cpp:2857-2858
@@ +2856,4 @@
+  if (Tok.isOneOf(tok::annot_module_include,
+                  tok::annot_module_begin,
+                  tok::annot_module_end)) {
+    Actions.checkModuleImportContext(reinterpret_cast<Module *>(
----------------
For module_end, we probably still want to give the "missing '}'" diagnostic, because that's almost certainly the problem if we reach the end of a module while still within a function or namespace. Maybe pass a flag into `checkModuleImportContext` to say whether you're entering or leaving the module?

================
Comment at: lib/Parse/ParseDeclCXX.cpp:2861-2862
@@ +2860,4 @@
+        Tok.getAnnotationValue()), Tok.getLocation(), Actions.CurContext);
+    ConsumeToken();
+    return DeclGroupPtrTy();
+  }
----------------
This is not a good way to recover from the problem: for that, we should probably do what we used to do, and bail out to the top level.


http://reviews.llvm.org/D11844