rsmith added a comment.
Sorry for the delay. I'm generally happy with the direction of this patch; I
think this will substantially improve how we diagnose this set of problems.
================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1016
@@ -1015,2 +1015,3 @@
"expected ';' after module name">;
+def err_missing_before_module_end : Error<"expected %0 at the end of module">;
}
----------------
I would drop the "the" in this error.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7774
@@ -7771,1 +7773,3 @@
"@import of module '%0' in implementation of '%1'; use #import">;
+def note_module_need_top_level : Note<"consider moving the import to top
level">;
+def note_module_need_textual : Note<"consider marking the header as textual">;
----------------
We'll already have said something like:
error: import of module 'foo' appears within class X
note: class X begins here
There are two likely situations here: either the header was intended to be
included textually, or the user forgot a `}` or similar and didn't expect for
the import to be inside the class. Either way, this note is not useful (either
they thought it *was* at the top level, or the note is not applicable).
How about dropping this note? The "begins here" note seems to have already
covered the interesting case.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7775
@@ -7772,1 +7774,3 @@
+def note_module_need_top_level : Note<"consider moving the import to top
level">;
+def note_module_need_textual : Note<"consider marking the header as textual">;
}
----------------
Textual headers are not the common case, so "consider" seems too strong; how
about weakening this to something like:
"if this header is intended to be included textually, mark it 'textual' in
the module map"
================
Comment at: lib/Parse/ParseDeclCXX.cpp:213
@@ -212,2 +212,3 @@
if (index == Ident.size()) {
- while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
+ while (tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
+ Tok.isNot(tok::eof)) {
----------------
Move this `try...` call to the end of the `&&` here and elsewhere; it's much
more likely that we'll hit an `r_brace`.
================
Comment at: lib/Parse/Parser.cpp:2023-2024
@@ +2022,4 @@
+/// be found.
+/// \returns true if the recover was successful and parsing may be continued,
or
+/// false if parser must bail out to top level and handle the token there.
+bool Parser::tryParseMisplacedModuleImport() {
----------------
This is the opposite of how all the other `try` functions behave: they return
`true` if they consumed the thing they were asked to consume.
================
Comment at: lib/Parse/Parser.cpp:2025-2027
@@ +2024,5 @@
+/// false if parser must bail out to top level and handle the token there.
+bool Parser::tryParseMisplacedModuleImport() {
+ while (true) {
+ switch (Tok.getKind()) {
+ case tok::annot_module_end:
----------------
Refactor this as follows:
Rename this to `parseMisplacedModuleImport`. Add an inline
`tryParseMisplacedModuleImport` to Parser.h, which checks if the current token
is an EOM token. If it is, it calls this function. If it is not, it returns
immediately.
The purpose of having these `try` functions with thin wrappers in the .h file
is so that we can inline the cheap "are we looking at the right kind of token"
check (and allow it to be combined with other nearby checks on the token kind).
================
Comment at: lib/Sema/SemaDecl.cpp:14361-14366
@@ -14359,6 +14360,8 @@
if (!isa<TranslationUnitDecl>(DC)) {
- S.Diag(ImportLoc, diag::err_module_import_not_at_top_level)
- << M->getFullModuleName() << DC;
- S.Diag(cast<Decl>(DC)->getLocStart(),
- diag::note_module_import_not_at_top_level)
- << DC;
+ if (DC->isNamespace()) {
+ S.Diag(ImportLoc, diag::err_module_import_not_at_top_level)
+ << M->getFullModuleName() << DC;
+ S.Diag(cast<Decl>(DC)->getLocStart(),
+ diag::note_module_import_not_at_top_level) << DC;
+ S.Diag(ImportLoc, diag::note_module_need_top_level);
+ } else {
----------------
It looks like this will currently produce repeated diagnostics for an import
within multiple namespaces:
module.map:
module Foo { module X { header "foo1.h" } module Y { header "foo2.h" } }
foo1.h:
namespace A {
namespace B {
#include "foo2.h"
}
}
I believe this will diagnose:
1) import of "Foo.Y" within "B"
2) expected } at end of "B"
3) import of "Foo.Y" within "A"
4) expected } at end of "A"
I think we should treat this case as fatal too.
================
Comment at: lib/Sema/SemaDecl.cpp:14372
@@ +14371,3 @@
+ diag::note_module_import_not_at_top_level) << DC;
+ S.Diag(ImportLoc, diag::note_module_need_textual);
+ }
----------------
I think we should only suggest changing the module map if our current module
and the module of the included file are within the same top-level module. It
would be unfortunate for us to suggest modifying, say, the libc module map if
we see some end user's code including a C library header in the wrong context.
You can check `M->getTopLevelModuleName()` against
`S.getLangOpts().CurrentModule` and `S.getLangOpts().ImplementationOfModule` to
see whether it's a submodule of the current module.
http://reviews.llvm.org/D11844
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
