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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
