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

[Richard Smith via Phabricator via cfe-commits]

rsmith 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<
----------------
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.)


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8710
+def note_redefinition_modules_same_file_modulemap : Note<
+	"consider adding '%0' as part of '%1' definition in">;
 }
----------------
This note ends in the middle of a sentence.


================
Comment at: lib/Sema/SemaDecl.cpp:3731
 
+void Sema::diagnoseRedefinition(SourceLocation Old, SourceLocation New) {
+  SourceManager &SrcMgr = getSourceManager();
----------------
Can you give this a name that suggests that it produces a note rather than a full diagnostic? `notePreviousDefinition`, maybe.


================
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()) {
----------------
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`).


================
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();
----------------
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.


================
Comment at: lib/Sema/SemaDecl.cpp:3763
+      }
+    } else { // Redefinitions caused by the lack of header guards.
+      Diag(IncludeLoc, diag::note_redefinition_same_file)
----------------
I don't think this should be an `else`. If both locations were textually included in the current translation, we should still produce the "missing include guard" diagnostic. Conversely, it'd seem reasonable to ask the preprocessor if the header has an include guard before claiming that it doesn't.


https://reviews.llvm.org/D28832