bruno marked 5 inline comments as done.
bruno added inline comments.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8708
+def note_redefinition_modules_same_file : Note<
+ "'%0' included multiple times, additional (likely non-modular) include
site in module '%1'">;
+def note_redefinition_modules_same_file_modulemap : Note<
----------------
rsmith wrote:
> If we can't be sure whether or not the other include side was a modular
> include, making a guess is probably not helpful. (In fact, I've seen this
> issue show up a lot more where the header is a modular header in both
> modules.)
After thinking more about this I agree: if used together with
"-Wnon-modular-include-in-module" and similars, it should become more evident
to the user what this is about.
================
Comment at: lib/Sema/SemaDecl.cpp:3747
+ // is confusing, try to get better diagnostics when modules is on.
+ auto OldModLoc = SrcMgr.getModuleImportLoc(Old);
+ if (!OldModLoc.first.isInvalid()) {
----------------
rsmith wrote:
> Rather than listing where the module was first imported (which could be
> unrelated to the problem), it would make more sense to list the location at
> which the previous declaration was made visible. You can get that by querying
> the `VisibleModuleSet` for the owning module of the definition and every
> other module in its merged definitions set (see
> `Sema::hasVisibleMergedDefinition`).
I tried this, but AFAIU the Decls coming from non-modular headers are usually
hidden, and since it has not been merged, which happens in the testcase because
it's a var redefinition error, then we have no import location to get
information from. Do you have a synthetic testcase in mind that I can use? I'm
happy to follow up with that (here or in a next patch).
================
Comment at: lib/Sema/SemaDecl.cpp:3755-3759
+ if (!Mod->DefinitionLoc.isInvalid())
+ Diag(Mod->DefinitionLoc,
+ diag::note_redefinition_modules_same_file_modulemap)
+ << SrcMgr.getFilename(SrcMgr.getSpellingLoc(Old)).str()
+ << Mod->getFullModuleName();
----------------
rsmith wrote:
> Is this ("add the header to the module map for some other module that
> happened to include it first") really generally good advice? People have a
> tendency to follow such guidance blindly.
Indeed, going to remove that.
https://reviews.llvm.org/D28832
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
