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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM. I'd like to make sure we try to use something better than the first import location for a module (that location is especially meaningless under `-fmodules-local-submodule-visibility`), but I'm happy for that (the big comment) to be dealt with as a follow-on change, and all the other comments here are minor, so feel free to commit after addressing those.



================
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">;
 }
----------------
rsmith wrote:
> This note ends in the middle of a sentence.
... this note still ends in the middle of a sentence.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4584
+def note_use_ifdef_guards :
+  Note <"unguarded header; consider using #ifdef guards or #pragma once">;
 
----------------
Nit: we generally put the `Note<` on the prior line.


================
Comment at: lib/Sema/SemaDecl.cpp:3594
+    notePreviousDefinition(Previous.getRepresentativeDecl()->getLocation(),
+                         New->getLocation());
     return New->setInvalidDecl();
----------------
Reindent.


================
Comment at: lib/Sema/SemaDecl.cpp:3812
+
+    if (!IncLoc.isInvalid()) {
+      if (Mod) {
----------------
Use `isValid` to avoid double-negative.


================
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()) {
----------------
bruno wrote:
> 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).
The `int c = 1;` testcase seems like it ought to be pretty rare (defining a variable in a header), and it might make more sense to say "hey, you probably didn't want a strong definition in a header at all" rather than pointing out the header was included twice.

Here's one pattern we've seen a few times that might be useful as a testcase:

== foo.h:
```
#ifndef FOO
#define FOO
#include "bar.h"
struct A {};
#endif
```

== bar.h:
```
#ifndef BAR
#define BAR
#include "foo.h"
#endif
```

== module.map:
```
module bar { header "bar.h" }
```

== x.c:
```
#include "foo.h"
```

[This results in us entering the FOO include guard, importing bar.h (including a definition of A) and then parsing another definition of A.]


https://reviews.llvm.org/D28832