Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
This revision was automatically updated to reflect the committed changes.
Closed by commit rL248085: [Modules] More descriptive diagnostics for misplaced
import directive (authored by sepavloff).
Changed prior to commit:
http://reviews.llvm.org/D11844?vs=35115&id=35161#toc
Repository:
rL LLVM
http://reviews.llvm.org/D11844
Files:
cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Parse/Parser.h
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Parse/ParseDecl.cpp
cfe/trunk/lib/Parse/ParseDeclCXX.cpp
cfe/trunk/lib/Parse/ParseStmt.cpp
cfe/trunk/lib/Parse/Parser.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/test/Modules/Inputs/misplaced/misplaced-a.h
cfe/trunk/test/Modules/Inputs/misplaced/misplaced-b.h
cfe/trunk/test/Modules/Inputs/misplaced/misplaced.modulemap
cfe/trunk/test/Modules/auto-module-import.m
cfe/trunk/test/Modules/extern_c.cpp
cfe/trunk/test/Modules/malformed.cpp
cfe/trunk/test/Modules/misplaced-1.cpp
cfe/trunk/test/Modules/misplaced-2.cpp
cfe/trunk/test/Modules/misplaced-3.cpp
cfe/trunk/test/Modules/misplaced-4.cpp
cfe/trunk/test/Modules/misplaced-5.c
Index: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
@@ -1013,6 +1013,7 @@
"expected a module name after module import">;
def err_module_expected_semi : Error<
"expected ';' after module name">;
+def err_missing_before_module_end : Error<"expected %0 at end of module">;
}
let CategoryName = "Generics Issue" in {
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7776,6 +7776,8 @@
"extern \"C\" language linkage specification begins here">;
def err_module_import_not_at_top_level : Error<
"import of module '%0' appears within %1">;
+def err_module_import_not_at_top_level_fatal : Error<
+ "import of module '%0' appears within %1">, DefaultFatal;
def note_module_import_not_at_top_level : Note<"%0 begins here">;
def err_module_self_import : Error<
"import of module '%0' appears within same top-level module '%1'">;
Index: cfe/trunk/include/clang/Parse/Parser.h
===
--- cfe/trunk/include/clang/Parse/Parser.h
+++ cfe/trunk/include/clang/Parse/Parser.h
@@ -2555,6 +2555,14 @@
//======//
// Modules
DeclGroupPtrTy ParseModuleImport(SourceLocation AtLoc);
+ bool parseMisplacedModuleImport();
+ bool tryParseMisplacedModuleImport() {
+tok::TokenKind Kind = Tok.getKind();
+if (Kind == tok::annot_module_begin || Kind == tok::annot_module_end ||
+Kind == tok::annot_module_include)
+ return parseMisplacedModuleImport();
+return false;
+ }
//======//
// C++11/G++: Type Traits [Type-Traits.html in the GCC manual]
Index: cfe/trunk/include/clang/Sema/Sema.h
===
--- cfe/trunk/include/clang/Sema/Sema.h
+++ cfe/trunk/include/clang/Sema/Sema.h
@@ -1799,6 +1799,10 @@
/// \brief The parser has left a submodule.
void ActOnModuleEnd(SourceLocation DirectiveLoc, Module *Mod);
+ /// \brief Check if module import may be found in the current context,
+ /// emit error if not.
+ void diagnoseMisplacedModuleImport(Module *M, SourceLocation ImportLoc);
+
/// \brief Create an implicit import of the given module at the given
/// source location, for error recovery, if possible.
///
Index: cfe/trunk/test/Modules/Inputs/misplaced/misplaced-b.h
===
--- cfe/trunk/test/Modules/Inputs/misplaced/misplaced-b.h
+++ cfe/trunk/test/Modules/Inputs/misplaced/misplaced-b.h
@@ -0,0 +1 @@
+int a;
\ No newline at end of file
Index: cfe/trunk/test/Modules/Inputs/misplaced/misplaced-a.h
===
--- cfe/trunk/test/Modules/Inputs/misplaced/misplaced-a.h
+++ cfe/trunk/test/Modules/Inputs/misplaced/misplaced-a.h
@@ -0,0 +1,5 @@
+namespace A {
+ namespace B { // expected-note{{namespace 'A::B' begins here}}
+#include "misplaced-b.h" // expected-error{{import of module 'Misplaced.Sub_B' appears within namespace 'A::B'}}
+ }
+}
Index: cfe/trunk/test/Modules/Inputs/misplaced/misplaced.modulemap
===
--- cfe/trunk/test/Modules/Inputs/misplaced/misplaced.modulemap
+++ cfe/trunk/test/Modules/Inputs/misplaced/misplaced.modulemap
@@ -0,0 +1,8 @@
+module Misplaced {
+ module S
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
rsmith added a comment. A separate patch for the ObjC cases is fine. http://reviews.llvm.org/D11844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
rsmith accepted this revision.
rsmith added a reviewer: rsmith.
Comment at: include/clang/Parse/Parser.h:2560-2562
@@ +2559,5 @@
+ bool tryParseMisplacedModuleImport() {
+tok::TokenKind Kind = Tok.getKind();
+if (Kind == tok::annot_module_begin || Kind == tok::annot_module_end ||
+Kind == tok::annot_module_include)
+ return parseMisplacedModuleImport();
Use `Tok.isOneOf` here.
http://reviews.llvm.org/D11844
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
sepavloff added a comment.
Thanks for helpful notes!
There are similar cases in Objective C. Should they be implemented in this
patch or separate patch is OK?
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">;
}
rsmith wrote:
> I would drop the "the" in this error.
Done.
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">;
silvas wrote:
> rsmith wrote:
> > 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.
> I think this makes sense. `error: ... import ... appears within ...` seems
> like sufficient information about the nature of the error.
The message is removed.
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">;
}
rsmith wrote:
> 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"
This message is also removed, as Sean proposed.
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)) {
rsmith wrote:
> Move this `try...` call to the end of the `&&` here and elsewhere; it's much
> more likely that we'll hit an `r_brace`.
Moved.
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() {
rsmith wrote:
> 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.
Meaning of the return value is inverted.
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:
rsmith wrote:
> 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).
Thank you for explanation. Fixed.
Comment at: lib/Sema/SemaDecl.cpp:14361-14366
@@ -14359,6 +14360,8 @@
if (!isa(DC)) {
-S.Diag(ImportLoc, diag::err_module_import_not_at_top_level)
- << M->getFullModuleName() << DC;
-S.Diag(cast(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(DC)->getLocStart(),
+ diag::note_module_import_not_at_top_level) << DC;
+ S.Diag(ImportLoc, diag::note_module_need_top_level);
+} else {
rsmith wrote:
> 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.
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
sepavloff updated this revision to Diff 35115.
sepavloff added a comment.
Updated patch.
http://reviews.llvm.org/D11844
Files:
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
lib/Parse/ParseDecl.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Parse/ParseStmt.cpp
lib/Parse/Parser.cpp
lib/Sema/SemaDecl.cpp
test/Modules/Inputs/misplaced/misplaced-a.h
test/Modules/Inputs/misplaced/misplaced-b.h
test/Modules/Inputs/misplaced/misplaced.modulemap
test/Modules/Inputs/misplaced2.cpp
test/Modules/Inputs/misplaced3.cpp
test/Modules/auto-module-import.m
test/Modules/extern_c.cpp
test/Modules/malformed.cpp
test/Modules/misplaced-1.cpp
test/Modules/misplaced-2.cpp
test/Modules/misplaced-3.cpp
test/Modules/misplaced-4.cpp
test/Modules/misplaced-5.c
Index: test/Modules/misplaced-5.c
===
--- /dev/null
+++ test/Modules/misplaced-5.c
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
+
+struct S1 { // expected-note{{'struct S1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within 'struct S1'}}
+}
Index: test/Modules/misplaced-4.cpp
===
--- /dev/null
+++ test/Modules/misplaced-4.cpp
@@ -0,0 +1,2 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -emit-module -fmodule-name=Misplaced -fmodules-cache-path=%t -x c++ -I %S/Inputs %S/Inputs/misplaced/misplaced.modulemap -verify
Index: test/Modules/misplaced-3.cpp
===
--- /dev/null
+++ test/Modules/misplaced-3.cpp
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
+
+class C1 { // expected-note{{'C1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within 'C1'}}
+}
Index: test/Modules/misplaced-2.cpp
===
--- /dev/null
+++ test/Modules/misplaced-2.cpp
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
+
+void func1() { // expected-note{{function 'func1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within function 'func1'}}
+}
Index: test/Modules/misplaced-1.cpp
===
--- /dev/null
+++ test/Modules/misplaced-1.cpp
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
+
+namespace N1 { // expected-note{{namespace 'N1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within namespace 'N1'}}
+}
Index: test/Modules/malformed.cpp
===
--- test/Modules/malformed.cpp
+++ test/Modules/malformed.cpp
@@ -12,20 +12,14 @@
#include STR(HEADER)
// CHECK-A: While building module 'malformed_a'
-// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} error: expected '}'
+// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} error: expected '}' at end of module
// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} note: to match this '{'
//
// CHECK-A: While building module 'malformed_a'
// CHECK-A: {{^}}Inputs/malformed/a2.h:1:{{.*}} error: extraneous closing brace
// CHECK-B: While building module 'malformed_b'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:2:{{.*}} error: expected '}'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:1:{{.*}} note: to match this '{'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:3:{{.*}} error: extraneous closing brace ('}')
-//
-// CHECK-B: While building module 'malformed_b'
-// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} error: redefinition of 'g'
-// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} note: previous definition is here
+// CHECK-B: {{^}}Inputs/malformed/b1.h:2:{{.*}} error: import of module 'malformed_b.b2' appears within 'S'
void test() { f(); }
// Test that we use relative paths to name files within an imported module.
Index: test/Modules/extern_c.cpp
===
--- test/Modules/extern_c.cpp
+++ test/Modules/extern_c.cpp
@@ -68,7 +68,7 @@
extern "C" {
#endif
int f;
-#if !defined(CXX_HEADER)
+#if !defined(CXX_HEADER) && !defined(NAMESPACE)
// expected-error@-2 {{redefinition of 'f' as different kind of symbol}}
// expected-note@c-header.h:1 {{previous}}
#endif
@@ -78,4 +78,6 @@
}
#endif
+#if !defined(NAMESPACE)
suppress_expected_no_diagnostics_error error_here; // expected-error {{}}
+#endif
Index: test/Modules/auto-module-import.m
===
--- test
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
silvas requested changes to this revision.
silvas added a reviewer: silvas.
This revision now requires changes to proceed.
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">;
rsmith wrote:
> 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.
I think this makes sense. `error: ... import ... appears within ...` seems like
sufficient information about the nature of the error.
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);
+}
rsmith wrote:
> 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.
From my experience, your suggested heuristic is insufficient; we definitely
need to cover the case that crosses top-level modules (or is from a .cpp file
to a module it imports); actually, that is the only case that I have run into
in practice.
In this patch, I think the right thing is to just not emit
`note_module_need_top_level` nor `note_module_need_textual`. A separate patch
can use a flag `-Wintial-modules-migration` (or whatever) to emit these notes a
bit more aggressively (the flag would be off by default).
Comment at: test/Modules/misplaced.cpp:1-12
@@ +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I
%S/Inputs %s -verify
+
+namespace N1 { // expected-note{{namespace 'N1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within
namespace 'N1'}} \
+// expected-note{{consider moving the import to top level}}
+}
+
+void func1() { // expected-note{{function 'func1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within
function 'func1'}} \
+// expected-note{{consider marking the header as textual}}
+}
This test does not cover the `appears within class` case.
http://reviews.llvm.org/D11844
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
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(DC)) {
-S.Diag(ImportLoc, diag::err_module_import_not_at_top_level)
- << M->getFullModuleName() << DC;
-S.Diag(cast(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(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.
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
sepavloff updated this revision to Diff 34673.
sepavloff added a comment.
Updated diagnostics according to Sean's notes ('textual module' -> 'textual
header')
http://reviews.llvm.org/D11844
Files:
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
lib/Parse/ParseDeclCXX.cpp
lib/Parse/ParseStmt.cpp
lib/Parse/Parser.cpp
lib/Sema/SemaDecl.cpp
test/Modules/auto-module-import.m
test/Modules/extern_c.cpp
test/Modules/malformed.cpp
test/Modules/misplaced.cpp
Index: test/Modules/misplaced.cpp
===
--- /dev/null
+++ test/Modules/misplaced.cpp
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
+
+namespace N1 { // expected-note{{namespace 'N1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within namespace 'N1'}} \
+// expected-note{{consider moving the import to top level}}
+}
+
+void func1() { // expected-note{{function 'func1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within function 'func1'}} \
+// expected-note{{consider marking the header as textual}}
+}
Index: test/Modules/malformed.cpp
===
--- test/Modules/malformed.cpp
+++ test/Modules/malformed.cpp
@@ -12,20 +12,14 @@
#include STR(HEADER)
// CHECK-A: While building module 'malformed_a'
-// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} error: expected '}'
+// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} error: expected '}' at the end of module
// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} note: to match this '{'
//
// CHECK-A: While building module 'malformed_a'
// CHECK-A: {{^}}Inputs/malformed/a2.h:1:{{.*}} error: extraneous closing brace
// CHECK-B: While building module 'malformed_b'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:2:{{.*}} error: expected '}'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:1:{{.*}} note: to match this '{'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:3:{{.*}} error: extraneous closing brace ('}')
-//
-// CHECK-B: While building module 'malformed_b'
-// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} error: redefinition of 'g'
-// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} note: previous definition is here
+// CHECK-B: {{^}}Inputs/malformed/b1.h:2:{{.*}} error: import of module 'malformed_b.b2' appears within 'S'
void test() { f(); }
// Test that we use relative paths to name files within an imported module.
Index: test/Modules/extern_c.cpp
===
--- test/Modules/extern_c.cpp
+++ test/Modules/extern_c.cpp
@@ -39,9 +39,11 @@
#if defined(EXTERN_C) && !defined(EXTERN_CXX) && defined(CXX_HEADER)
// expected-error@-3 {{import of C++ module 'cxx_library' appears within extern "C" language linkage specification}}
// expected-note@-17 {{extern "C" language linkage specification begins here}}
+// expected-note@-5 {{consider moving the import to top level}}
#elif defined(NAMESPACE)
-// expected-error-re@-6 {{import of module '{{c_library.inner|cxx_library}}' appears within namespace 'M'}}
-// expected-note@-24 {{namespace 'M' begins here}}
+// expected-error-re@-7 {{import of module '{{c_library.inner|cxx_library}}' appears within namespace 'M'}}
+// expected-note@-25 {{namespace 'M' begins here}}
+// expected-note@-9 {{consider moving the import to top level}}
#endif
#ifdef EXTERN_CXX
Index: test/Modules/auto-module-import.m
===
--- test/Modules/auto-module-import.m
+++ test/Modules/auto-module-import.m
@@ -83,6 +83,8 @@
return not_in_module;
}
-void includeNotAtTopLevel() { // expected-note {{to match this '{'}}
- #include // expected-warning {{treating #include as an import}} expected-error {{expected '}'}}
-} // expected-error {{extraneous closing brace}}
+void includeNotAtTopLevel() { // expected-note {{function 'includeNotAtTopLevel' begins here}}
+ #include // expected-warning {{treating #include as an import}} \
+ expected-error {{import of module 'NoUmbrella.A' appears within function 'includeNotAtTopLevel'}} \
+ expected-note {{consider marking the header as textual}}
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -14345,6 +14345,7 @@
S.Diag(ImportLoc, diag::err_module_import_in_extern_c)
<< M->getFullModuleName();
S.Diag(LSD->getLocStart(), diag::note_module_import_in_extern_c);
+S.Diag(ImportLoc, diag::note_module_need_top_level);
return;
}
break;
@@ -14357,14 +14358,26 @@
while (isa(DC))
DC = DC->getParent();
if (!isa(DC)) {
-S.Diag(I
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
silvas added a comment. ping? http://reviews.llvm.org/D11844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
sepavloff added a subscriber: sepavloff. sepavloff added a comment. Ping. Thanks, --Serge http://reviews.llvm.org/D11844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
Ping.
Thanks,
--Serge
2015-08-22 3:15 GMT+06:00 Sean Silva :
> silvas added inline comments.
>
>
> Comment at: test/Modules/auto-module-import.m:89
> @@ +88,3 @@
> + expected-error {{import of module
> 'NoUmbrella.A' appears within function 'includeNotAtTopLevel'}} \
> + expected-note {{consider marking the module
> as textual}}
> +}
>
> I think what this means is marking the *header* as textual. So I would
> suggest the wording "consider marking the header as textual".
>
>
> http://reviews.llvm.org/D11844
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
silvas added inline comments.
Comment at: test/Modules/auto-module-import.m:89
@@ +88,3 @@
+ expected-error {{import of module 'NoUmbrella.A'
appears within function 'includeNotAtTopLevel'}} \
+ expected-note {{consider marking the module as
textual}}
+}
I think what this means is marking the *header* as textual. So I would suggest
the wording "consider marking the header as textual".
http://reviews.llvm.org/D11844
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
On Wed, Aug 12, 2015 at 6:05 PM, Richard Smith wrote: > On Wed, Aug 12, 2015 at 6:00 PM, Sean Silva wrote: > >> >> >> On Wed, Aug 12, 2015 at 2:43 PM, Richard Smith >> wrote: >> >>> On Wed, Aug 12, 2015 at 12:08 AM, Sean Silva >>> wrote: >>> 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. >>> >>> >>> I think your observations may be biased by your current experiences >>> modularizing existing code, and not representative of the complete >>> development cycle. Modularization is a one-time effort; maintaining the >>> code after modularization is a continuous process. I think it's *far* more >>> likely that a header listed in your module map was expected to be modular, >>> and that a brace mismatch within that file is unintentional and due to a >>> missing brace somewhere. >>> >> >> I don't think so. That implies that the inclusion is not at the top of >> the file, which is extremely unlikely in a modular codebase. >> > > This also happens when there's a missing brace at the end of your modular > header, which is almost certainly the most common way to hit this problem > in an already-modularized codebase. > I think that handling the missing brace at end of module differently should be doable without disturbing the notes that Serge already has here. > And it happens in codebases that use a mixture of modular and non-modular > headers, where there's no reason to expect all the modular includes are > before the non-modular ones. > We could check that the opening brace was in the file containing the import to avoid misdiagnosing a missing brace in a textual header. Richard, are these cases that you want Serge to deal with in the present patch? -- Sean Silva > > >> 125993 #include lines. >> >> >>> >>> However, we should aim to provide diagnostics that are helpful for >>> either case. >>> >>> 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 com
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
sepavloff updated this revision to Diff 32085.
sepavloff added a comment.
Updated patch
Thanks to all for fruitful discussion!
The new version tries to addresses review notes. It differs from the previous
version in:
- Method tryParseMisplacedModuleImport now depends only on current token and is
used in loop condition. Probably a better name for it is something like
skipMisplacedModuleImport.
- BalancedDelimiterTracker is updated so that it prints different message if
module ended before closing bracket has been seen.
- If annot_module_begin is seen where it must not, recovery depends on context.
Inside a namespace the module is imported as a recovery, a note suggesting to
move import to top level is issued. Inside a class of a compound statement
fatal error issued, there is no way to handle this case at upper level.
http://reviews.llvm.org/D11844
Files:
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
lib/Parse/ParseDeclCXX.cpp
lib/Parse/ParseStmt.cpp
lib/Parse/Parser.cpp
lib/Sema/SemaDecl.cpp
test/Modules/auto-module-import.m
test/Modules/extern_c.cpp
test/Modules/malformed.cpp
test/Modules/misplaced.cpp
Index: test/Modules/misplaced.cpp
===
--- /dev/null
+++ test/Modules/misplaced.cpp
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
+
+namespace N1 { // expected-note{{namespace 'N1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within namespace 'N1'}} \
+// expected-note{{consider moving the import to top level}}
+}
+
+void func1() { // expected-note{{function 'func1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within function 'func1'}} \
+// expected-note{{consider marking the module as textual}}
+}
Index: test/Modules/malformed.cpp
===
--- test/Modules/malformed.cpp
+++ test/Modules/malformed.cpp
@@ -12,20 +12,14 @@
#include STR(HEADER)
// CHECK-A: While building module 'malformed_a'
-// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} error: expected '}'
+// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} error: expected '}' at the end of module
// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} note: to match this '{'
//
// CHECK-A: While building module 'malformed_a'
// CHECK-A: {{^}}Inputs/malformed/a2.h:1:{{.*}} error: extraneous closing brace
// CHECK-B: While building module 'malformed_b'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:2:{{.*}} error: expected '}'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:1:{{.*}} note: to match this '{'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:3:{{.*}} error: extraneous closing brace ('}')
-//
-// CHECK-B: While building module 'malformed_b'
-// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} error: redefinition of 'g'
-// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} note: previous definition is here
+// CHECK-B: {{^}}Inputs/malformed/b1.h:2:{{.*}} error: import of module 'malformed_b.b2' appears within 'S'
void test() { f(); }
// Test that we use relative paths to name files within an imported module.
Index: test/Modules/extern_c.cpp
===
--- test/Modules/extern_c.cpp
+++ test/Modules/extern_c.cpp
@@ -39,9 +39,11 @@
#if defined(EXTERN_C) && !defined(EXTERN_CXX) && defined(CXX_HEADER)
// expected-error@-3 {{import of C++ module 'cxx_library' appears within extern "C" language linkage specification}}
// expected-note@-17 {{extern "C" language linkage specification begins here}}
+// expected-note@-5 {{consider moving the import to top level}}
#elif defined(NAMESPACE)
-// expected-error-re@-6 {{import of module '{{c_library.inner|cxx_library}}' appears within namespace 'M'}}
-// expected-note@-24 {{namespace 'M' begins here}}
+// expected-error-re@-7 {{import of module '{{c_library.inner|cxx_library}}' appears within namespace 'M'}}
+// expected-note@-25 {{namespace 'M' begins here}}
+// expected-note@-9 {{consider moving the import to top level}}
#endif
#ifdef EXTERN_CXX
Index: test/Modules/auto-module-import.m
===
--- test/Modules/auto-module-import.m
+++ test/Modules/auto-module-import.m
@@ -83,6 +83,8 @@
return not_in_module;
}
-void includeNotAtTopLevel() { // expected-note {{to match this '{'}}
- #include // expected-warning {{treating #include as an import}} expected-error {{expected '}'}}
-} // expected-error {{extraneous closing brace}}
+void includeNotAtTopLevel() { // expected-note {{function 'includeNotAtTopLevel' begins here}}
+ #include // expected-warning {{treating #include as an import}} \
+ expected-error {{import of module 'NoUmbrella.A'
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
On Wed, Aug 12, 2015 at 6:05 PM, Richard Smith wrote: > On Wed, Aug 12, 2015 at 6:00 PM, Sean Silva wrote: > >> >> >> On Wed, Aug 12, 2015 at 2:43 PM, Richard Smith >> wrote: >> >>> On Wed, Aug 12, 2015 at 12:08 AM, Sean Silva >>> wrote: >>> 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. >>> >>> >>> I think your observations may be biased by your current experiences >>> modularizing existing code, and not representative of the complete >>> development cycle. Modularization is a one-time effort; maintaining the >>> code after modularization is a continuous process. I think it's *far* more >>> likely that a header listed in your module map was expected to be modular, >>> and that a brace mismatch within that file is unintentional and due to a >>> missing brace somewhere. >>> >> >> I don't think so. That implies that the inclusion is not at the top of >> the file, which is extremely unlikely in a modular codebase. >> > > This also happens when there's a missing brace at the end of your modular > header, which is almost certainly the most common way to hit this problem > in an already-modularized codebase. And it happens in codebases that use a > mixture of modular and non-modular headers, where there's no reason to > expect all the modular includes are before the non-modular ones. > :lets-do-both.jpg: Do we have a way to guard just notes behind warning flags? Maybe "-Winitial-module-map" or something could enable the "assume this module map might have errors" heuristics (which in my experience are highly reliable for that use case). We can document to users to turn this on when initially modularizing. -- Sean Silva > > >> 125993 #include lines. >> >> >>> >>> However, we should aim to provide diagnostics that are helpful for >>> either case. >>> >>> 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.
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
On Wed, Aug 12, 2015 at 6:00 PM, Sean Silva wrote: > > > On Wed, Aug 12, 2015 at 2:43 PM, Richard Smith > wrote: > >> On Wed, Aug 12, 2015 at 12:08 AM, Sean Silva >> wrote: >> >>> 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. >> >> >> I think your observations may be biased by your current experiences >> modularizing existing code, and not representative of the complete >> development cycle. Modularization is a one-time effort; maintaining the >> code after modularization is a continuous process. I think it's *far* more >> likely that a header listed in your module map was expected to be modular, >> and that a brace mismatch within that file is unintentional and due to a >> missing brace somewhere. >> > > I don't think so. That implies that the inclusion is not at the top of the > file, which is extremely unlikely in a modular codebase. > This also happens when there's a missing brace at the end of your modular header, which is almost certainly the most common way to hit this problem in an already-modularized codebase. And it happens in codebases that use a mixture of modular and non-modular headers, where there's no reason to expect all the modular includes are before the non-modular ones. > 125993 #include lines. > > >> >> However, we should aim to provide diagnostics that are helpful for either >> case. >> >> 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 >>> >>> >>> >>> >> > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
On Wed, Aug 12, 2015 at 2:43 PM, Richard Smith wrote: > On Wed, Aug 12, 2015 at 12:08 AM, Sean Silva > wrote: > >> 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. > > > I think your observations may be biased by your current experiences > modularizing existing code, and not representative of the complete > development cycle. Modularization is a one-time effort; maintaining the > code after modularization is a continuous process. I think it's *far* more > likely that a header listed in your module map was expected to be modular, > and that a brace mismatch within that file is unintentional and due to a > missing brace somewhere. > I don't think so. That implies that the inclusion is not at the top of the file, which is extremely unlikely in a modular codebase. 125993 #include lines. > > However, we should aim to provide diagnostics that are helpful for either > case. > > 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 >> >> >> >> > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
On Wed, Aug 12, 2015 at 6:00 PM, Sean Silva wrote: > > > On Wed, Aug 12, 2015 at 2:43 PM, Richard Smith > wrote: > >> On Wed, Aug 12, 2015 at 12:08 AM, Sean Silva >> wrote: >> >>> 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. >> >> >> I think your observations may be biased by your current experiences >> modularizing existing code, and not representative of the complete >> development cycle. Modularization is a one-time effort; maintaining the >> code after modularization is a continuous process. I think it's *far* more >> likely that a header listed in your module map was expected to be modular, >> and that a brace mismatch within that file is unintentional and due to a >> missing brace somewhere. >> > > I don't think so. That implies that the inclusion is not at the top of the > file, which is extremely unlikely in a modular codebase. > > 125993 #include lines. > Oops, apparently I accidentally hit send... I'm gathering some real statistics on this. -- Sean Silva > > >> >> However, we should aim to provide diagnostics that are helpful for either >> case. >> >> 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 >>> >>> >>> >>> >> > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
On Wed, Aug 12, 2015 at 12:08 AM, Sean Silva wrote: > 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. I think your observations may be biased by your current experiences modularizing existing code, and not representative of the complete development cycle. Modularization is a one-time effort; maintaining the code after modularization is a continuous process. I think it's *far* more likely that a header listed in your module map was expected to be modular, and that a brace mismatch within that file is unintentional and due to a missing brace somewhere. However, we should aim to provide diagnostics that are helpful for either case. 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 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
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(
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
sepavloff updated this revision to Diff 31831.
sepavloff added a comment.
Fixed module boundary treatment.
This version must fix problems in the treatment of annot_modulbegin and
annot_module_end. Error recover for the token annot_module_include found
in wrong context can be made as previously, just by ignoring it. To handle
token 'annot_module_end' parser bails out to upper level, where the module
end can be processed, it will complain on missing '}' etc. As for
'annot_module_start', now parser tries to recover by skipping all content
till matching 'annot_module_end'.
Also made changes according to reviewer's notes.
http://reviews.llvm.org/D11844
Files:
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
lib/Parse/ParseDeclCXX.cpp
lib/Parse/ParseStmt.cpp
lib/Parse/Parser.cpp
lib/Sema/SemaDecl.cpp
test/Modules/auto-module-import.m
test/Modules/malformed.cpp
test/Modules/misplaced.cpp
Index: test/Modules/misplaced.cpp
===
--- /dev/null
+++ test/Modules/misplaced.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
+
+namespace N1 { // expected-note{{namespace 'N1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within namespace 'N1'}}
+}
+
+void func1() { // expected-note{{function 'func1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within function 'func1'}}
+}
+
+struct S1 { // expected-note{{'S1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within 'S1'}}
+};
Index: test/Modules/malformed.cpp
===
--- test/Modules/malformed.cpp
+++ test/Modules/malformed.cpp
@@ -12,20 +12,14 @@
#include STR(HEADER)
// CHECK-A: While building module 'malformed_a'
+// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} error: unexpected module end
// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} error: expected '}'
// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} note: to match this '{'
//
// CHECK-A: While building module 'malformed_a'
-// CHECK-A: {{^}}Inputs/malformed/a2.h:1:{{.*}} error: extraneous closing brace
// CHECK-B: While building module 'malformed_b'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:2:{{.*}} error: expected '}'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:1:{{.*}} note: to match this '{'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:3:{{.*}} error: extraneous closing brace ('}')
-//
-// CHECK-B: While building module 'malformed_b'
-// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} error: redefinition of 'g'
-// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} note: previous definition is here
+// CHECK-B: {{^}}Inputs/malformed/b1.h:2:{{.*}} error: submodule cannot be started here
void test() { f(); }
// Test that we use relative paths to name files within an imported module.
Index: test/Modules/auto-module-import.m
===
--- test/Modules/auto-module-import.m
+++ test/Modules/auto-module-import.m
@@ -83,6 +83,6 @@
return not_in_module;
}
-void includeNotAtTopLevel() { // expected-note {{to match this '{'}}
- #include // expected-warning {{treating #include as an import}} expected-error {{expected '}'}}
-} // expected-error {{extraneous closing brace}}
+void includeNotAtTopLevel() { // expected-note {{function 'includeNotAtTopLevel' begins here}}
+ #include // expected-warning {{treating #include as an import}} expected-error {{import of module 'NoUmbrella.A' appears within function 'includeNotAtTopLevel'}}
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -14274,6 +14274,10 @@
}
}
+void Sema::diagnoseMisplacedModuleImport(Module *M, SourceLocation ImportLoc) {
+ return checkModuleImportContext(*this, M, ImportLoc, CurContext);
+}
+
DeclResult Sema::ActOnModuleImport(SourceLocation AtLoc,
SourceLocation ImportLoc,
ModuleIdPath Path) {
Index: lib/Parse/Parser.cpp
===
--- lib/Parse/Parser.cpp
+++ lib/Parse/Parser.cpp
@@ -1989,6 +1989,43 @@
return Actions.ConvertDeclToDeclGroup(Import.get());
}
+/// \brief Try recover parser when module annotation appears where it must not
+/// 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() {
+ switch (Tok.getKind()) {
+ case tok::annot_module_end:
+Diag(Tok, diag::err_unexpected_module_end);
+break;
+ case tok::annot_module_begin: {
+Diag(Tok, diag::err_unexpected_module_start
Re: [PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
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(
+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(
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
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive
sepavloff created this revision.
sepavloff added a subscriber: cfe-commits.
If an import directive was put into wrong context, the error message was
obscure,
complaining on misbalanced braces. To get more descriptive messages, annotation
tokens related to modules are processed where they must not be seen.
http://reviews.llvm.org/D11844
Files:
include/clang/Sema/Sema.h
lib/Parse/ParseDeclCXX.cpp
lib/Parse/ParseStmt.cpp
lib/Parse/Parser.cpp
lib/Sema/SemaDecl.cpp
test/Modules/auto-module-import.m
test/Modules/malformed.cpp
test/Modules/misplaced.cpp
Index: test/Modules/misplaced.cpp
===
--- /dev/null
+++ test/Modules/misplaced.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
+
+namespace N1 { // expected-note{{namespace 'N1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within namespace 'N1'}}
+}
+
+void func1() { // expected-note{{function 'func1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within function 'func1'}}
+}
+
+struct S1 { // expected-note{{'S1' begins here}}
+#include "dummy.h" // expected-error{{import of module 'dummy' appears within 'S1'}}
+};
Index: test/Modules/malformed.cpp
===
--- test/Modules/malformed.cpp
+++ test/Modules/malformed.cpp
@@ -12,20 +12,18 @@
#include STR(HEADER)
// CHECK-A: While building module 'malformed_a'
-// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} error: expected '}'
-// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} note: to match this '{'
+// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} error: import of module 'malformed_a.a1' appears within function
+// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} note: function 'f' begins here
//
// CHECK-A: While building module 'malformed_a'
-// CHECK-A: {{^}}Inputs/malformed/a2.h:1:{{.*}} error: extraneous closing brace
// CHECK-B: While building module 'malformed_b'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:2:{{.*}} error: expected '}'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:1:{{.*}} note: to match this '{'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:3:{{.*}} error: extraneous closing brace ('}')
+// CHECK-B: {{^}}Inputs/malformed/b1.h:2:{{.*}} error: import of module 'malformed_b.b2' appears within 'S'
+// CHECK-B: {{^}}Inputs/malformed/b1.h:1:{{.*}} note: 'S' begins here
//
// CHECK-B: While building module 'malformed_b'
-// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} error: redefinition of 'g'
-// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} note: previous definition is here
+// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} error: import of module 'malformed_b.b2' appears within 'S'
+// CHECK-B: {{^}}Inputs/malformed/b1.h:1:{{.*}} note: 'S' begins here
void test() { f(); }
// Test that we use relative paths to name files within an imported module.
Index: test/Modules/auto-module-import.m
===
--- test/Modules/auto-module-import.m
+++ test/Modules/auto-module-import.m
@@ -83,6 +83,6 @@
return not_in_module;
}
-void includeNotAtTopLevel() { // expected-note {{to match this '{'}}
- #include // expected-warning {{treating #include as an import}} expected-error {{expected '}'}}
-} // expected-error {{extraneous closing brace}}
+void includeNotAtTopLevel() { // expected-note {{function 'includeNotAtTopLevel' begins here}}
+ #include // expected-warning {{treating #include as an import}} expected-error {{import of module 'NoUmbrella.A' appears within function 'includeNotAtTopLevel'}}
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -14244,16 +14244,15 @@
return New;
}
-static void checkModuleImportContext(Sema &S, Module *M,
- SourceLocation ImportLoc,
- DeclContext *DC) {
+void Sema::checkModuleImportContext(Module *M, SourceLocation ImportLoc,
+DeclContext *DC) {
if (auto *LSD = dyn_cast(DC)) {
switch (LSD->getLanguage()) {
case LinkageSpecDecl::lang_c:
if (!M->IsExternC) {
-S.Diag(ImportLoc, diag::err_module_import_in_extern_c)
+Diag(ImportLoc, diag::err_module_import_in_extern_c)
<< M->getFullModuleName();
-S.Diag(LSD->getLocStart(), diag::note_module_import_in_extern_c);
+Diag(LSD->getLocStart(), diag::note_module_import_in_extern_c);
return;
}
break;
@@ -14266,10 +14265,10 @@
while (isa(DC))
DC = DC->getParent();
if (!isa(DC)) {
-S.Diag(ImportLoc, diag::err_module_import_not_at_top_level)
+Diag(ImportLoc, diag::err_module_import_not_at_top_level)
<< M->getFullModuleName() << DC;
-S.Diag(cas
