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