[PATCH] D28832: Improve redefinition errors pointing to the same header.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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