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
