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<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 {
----------------
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.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.
Yes, the messages were as you described. Now import inside a namespace is also
a fatal error. Test `misplaced-4.cpp` checks this case.
================
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);
+ }
----------------
silvas wrote:
> 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).
As Sean proposed, these are not emitted.
================
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}}
+}
----------------
silvas wrote:
> This test does not cover the `appears within class` case.
This case was checked in `malformed.cpp` but for the sake of consistency
separate test was added.
http://reviews.llvm.org/D11844
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
