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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
