[PATCH] D28832: Improve redefinition errors pointing to the same header.
This revision was automatically updated to reflect the committed changes. Closed by commit rL302765: [Sema] Improve redefinition errors pointing to the same header (authored by bruno). Changed prior to commit: https://reviews.llvm.org/D28832?vs=95369=98584#toc Repository: rL LLVM https://reviews.llvm.org/D28832 Files: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td cfe/trunk/include/clang/Sema/Sema.h cfe/trunk/lib/Sema/SemaDecl.cpp cfe/trunk/test/Modules/Inputs/SameHeader/A.h cfe/trunk/test/Modules/Inputs/SameHeader/B.h cfe/trunk/test/Modules/Inputs/SameHeader/C.h cfe/trunk/test/Modules/Inputs/SameHeader/module.modulemap cfe/trunk/test/Modules/redefinition-same-header.m cfe/trunk/test/Sema/redefinition-same-header.c cfe/trunk/test/SemaCXX/modules-ts.cppm Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td === --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td @@ -4609,6 +4609,8 @@ "cannot add 'abi_tag' attribute in a redeclaration">; def err_new_abi_tag_on_redeclaration : Error< "'abi_tag' %0 missing in original declaration">; +def note_use_ifdef_guards : Note< + "unguarded header; consider using #ifdef guards or #pragma once">; def note_deleted_dtor_no_operator_delete : Note< "virtual destructor requires an unambiguous, accessible 'operator delete'">; @@ -,6 +8890,13 @@ InGroup>; def note_equivalent_internal_linkage_decl : Note< "declared here%select{ in module '%1'|}0">; + +def note_redefinition_modules_same_file : Note< + "'%0' included multiple times, additional include site in header from module '%1'">; +def note_redefinition_modules_same_file_modulemap : Note< + "consider adding '%0' as part of '%1' definition">; +def note_redefinition_include_same_file : Note< + "'%0' included multiple times, additional include site here">; } let CategoryName = "Coroutines Issue" in { Index: cfe/trunk/include/clang/Sema/Sema.h === --- cfe/trunk/include/clang/Sema/Sema.h +++ cfe/trunk/include/clang/Sema/Sema.h @@ -2353,6 +2353,7 @@ void MergeVarDeclTypes(VarDecl *New, VarDecl *Old, bool MergeTypeWithOld); void MergeVarDeclExceptionSpecs(VarDecl *New, VarDecl *Old); bool checkVarDeclRedefinition(VarDecl *OldDefn, VarDecl *NewDefn); + void notePreviousDefinition(SourceLocation Old, SourceLocation New); bool MergeCXXFunctionDecl(FunctionDecl *New, FunctionDecl *Old, Scope *S); // AssignmentAction - This is used by all the assignment diagnostic functions Index: cfe/trunk/test/SemaCXX/modules-ts.cppm === --- cfe/trunk/test/SemaCXX/modules-ts.cppm +++ cfe/trunk/test/SemaCXX/modules-ts.cppm @@ -17,7 +17,8 @@ int n; #if TEST >= 2 // expected-error@-2 {{redefinition of '}} -// expected-note@-3 {{previous}} +// expected-note@-3 {{unguarded header; consider using #ifdef guards or #pragma once}} +// expected-note...@modules-ts.cppm:1 {{'{{.*}}/modules-ts.cppm' included multiple times, additional include site here}} #endif #if TEST == 0 Index: cfe/trunk/test/Modules/redefinition-same-header.m === --- cfe/trunk/test/Modules/redefinition-same-header.m +++ cfe/trunk/test/Modules/redefinition-same-header.m @@ -0,0 +1,20 @@ +// RUN: rm -rf %t.tmp +// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SameHeader -fmodules \ +// RUN: -fimplicit-module-maps -fmodules-cache-path=%t.tmp %s -verify + +// expected-error@Inputs/SameHeader/C.h:3 {{redefinition of 'c'}} +// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}} +// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}} +// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}} + +// expected-error@Inputs/SameHeader/C.h:5 {{redefinition of 'aaa'}} +// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}} +// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}} +// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}} + +// expected-error@Inputs/SameHeader/C.h:9 {{redefinition of 'fd_set'}} +// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}} +// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}} +// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}} +#include "A.h" // maps to a modular +#include "C.h" // textual include Index:
[PATCH] D28832: Improve redefinition errors pointing to the same header.
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
[PATCH] D28832: Improve redefinition errors pointing to the same header.
bruno added a comment. Ping https://reviews.llvm.org/D28832 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28832: Improve redefinition errors pointing to the same header.
v.g.vassilev added a comment. @bruno, ok, sounds good. https://reviews.llvm.org/D28832 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28832: Improve redefinition errors pointing to the same header.
bruno added a comment. @v.g.vassilev your testcase is interesting but it's unrelated to this specific improvement. The error message it's triggered by (a) the decl name not being found because `kROOTD_OPEN` is hidden, (b) type transforms looks for an alternative (c) `Sema::diagnoseMissingImport` is called and produces the error you're seeing. I can fix this one too once we decide what we consider a good diagnostic here, let's iterate in http://bugs.llvm.org/show_bug.cgi?id=32670 https://reviews.llvm.org/D28832 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28832: Improve redefinition errors pointing to the same header.
bruno updated this revision to Diff 95369. bruno marked 2 inline comments as done. bruno added a comment. Updated the patch after Richard's comments: Before -- In file included from x.c:2: Inputs4/C.h:3:5: error: redefinition of 'c' int c = 1; ^ In module 'X' imported from x.c:1: Inputs4/C.h:3:5: note: previous definition is here int c = 1; ^ 1 error generated. After - In file included from x.c:2: Inputs4/C.h:3:5: error: redefinition of 'c' int c = 1; ^ In module 'X' imported from x.c:1: Inputs4/B.h:3:10: note: 'Inputs4/C.h' included multiple times, additional include site in header from module 'X.B' #include "C.h" ^ Inputs4/module.modulemap:6:10: note: X.B defined here module B { ^ x.c:2:10: note: 'Inputs4/C.h' included multiple times, additional include site here #include "C.h" // textual include ^ 1 error generated. https://reviews.llvm.org/D28832 Files: include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaDecl.cpp test/Modules/Inputs/SameHeader/A.h test/Modules/Inputs/SameHeader/B.h test/Modules/Inputs/SameHeader/C.h test/Modules/Inputs/SameHeader/module.modulemap test/Modules/redefinition-same-header.m test/Sema/redefinition-same-header.c Index: test/Sema/redefinition-same-header.c === --- /dev/null +++ test/Sema/redefinition-same-header.c @@ -0,0 +1,14 @@ +// RUN: rm -rf %t +// RUN: mkdir -p %t +// RUN: echo 'int yyy = 42;' > %t/a.h +// RUN: %clang_cc1 -fsyntax-only %s -I%t -verify + +// expected-error@a.h:1 {{redefinition of 'yyy'}} +// expected-note@a.h:1 {{unguarded header; consider using #ifdef guards or #pragma once}} +// expected-note-re@redefinition-same-header.c:11 {{'{{.*}}/a.h' included multiple times, additional include site here}} +// expected-note-re@redefinition-same-header.c:12 {{'{{.*}}/a.h' included multiple times, additional include site here}} + +#include "a.h" +#include "a.h" + +int foo() { return yyy; } Index: test/Modules/redefinition-same-header.m === --- /dev/null +++ test/Modules/redefinition-same-header.m @@ -0,0 +1,20 @@ +// RUN: rm -rf %t.tmp +// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SameHeader -fmodules \ +// RUN: -fimplicit-module-maps -fmodules-cache-path=%t.tmp %s -verify + +// expected-error@Inputs/SameHeader/C.h:3 {{redefinition of 'c'}} +// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}} +// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}} +// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}} + +// expected-error@Inputs/SameHeader/C.h:5 {{redefinition of 'aaa'}} +// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}} +// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}} +// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}} + +// expected-error@Inputs/SameHeader/C.h:9 {{redefinition of 'fd_set'}} +// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}} +// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}} +// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}} +#include "A.h" // maps to a modular +#include "C.h" // textual include Index: test/Modules/Inputs/SameHeader/module.modulemap === --- /dev/null +++ test/Modules/Inputs/SameHeader/module.modulemap @@ -0,0 +1,11 @@ +module X { + module A { +header "A.h" +export * + } + module B { +header "B.h" +export * + } + export * +} Index: test/Modules/Inputs/SameHeader/C.h === --- /dev/null +++ test/Modules/Inputs/SameHeader/C.h @@ -0,0 +1,12 @@ +#ifndef __C_h__ +#define __C_h__ +int c = 1; + +struct aaa { + int b; +}; + +typedef struct fd_set { + char c; +}; +#endif Index: test/Modules/Inputs/SameHeader/B.h === --- /dev/null +++ test/Modules/Inputs/SameHeader/B.h @@ -0,0 +1,4 @@ +#ifndef __B_h__ +#define __B_h__ +#include "C.h" +#endif Index: test/Modules/Inputs/SameHeader/A.h === --- /dev/null +++ test/Modules/Inputs/SameHeader/A.h @@ -0,0 +1,3 @@ +#ifndef __A_h__ +#define __A_h__ +#endif Index: lib/Sema/SemaDecl.cpp === --- lib/Sema/SemaDecl.cpp +++ lib/Sema/SemaDecl.cpp @@ -1969,7
[PATCH] D28832: Improve redefinition errors pointing to the same header.
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28832: Improve redefinition errors pointing to the same header.
v.g.vassilev added a comment. Here is an example that we recently had where we would be very happy to see better diagnostics: MessageTypes.h: #ifndef ROOT_MessageTypes #define ROOT_MessageTypes enum EMessageTypes { kROOTD_OPEN = 2004, //filename follows + mode }; #endif module.modulemap: module "libNet.so" { requires cplusplus module "TMessage.h" { header "TMessage.h" export * } module "TNetFile.h" { header "TNetFile.h" export * } export * } Output: TNetFile.cxx:1:10: remark: building module 'libNet.so' as '/home/teemperor/Downloads/merge_enum/pcms/3MNDY7KURXTXS/libNet.so-141H35WZHCMDE.pcm' [-Rmodule-build] #include "TNetFile.h" ^ TNetFile.cxx:1:10: remark: finished building module 'libNet.so' [-Rmodule-build] TNetFile.cxx:2:10: error: declaration of 'kROOTD_OPEN' must be imported from module 'libNet.so.TMessage.h' before it is required auto v = kROOTD_OPEN; ^ In module 'libNet.so' imported from TNetFile.cxx:1: ./MessageTypes.h:5:4: note: previous declaration is here kROOTD_OPEN = 2004, //filename follows + mode ^ 1 error generated. test.sh: #!/bin/bash CLANG="/opt/clang/inst/bin/clang" rm -rf ./pcms/ # Normal clang invocation #"$CLANG" -cc1 -triple x86_64-apple-macosx10.11.0 -fsyntax-only -I . -std=c++11 -x c++ TNetFile.cxx -nostdsysteminc # Modules clang invocation "$CLANG" -cc1 -triple x86_64-apple-macosx10.11.0 -fsyntax-only -I . -std=c++11 -fmodules -fimplicit-module-maps -fmodules-cache-path=./pcms/ -fcolor-diagnostics -fdiagnostics-show-option -fdiagnostics-show-note-include-stack -x c++ TNetFile.cxx -nostdsysteminc -Rmodule-build TMessage.h: #ifndef ROOT_TMessage #define ROOT_TMessage #include "MessageTypes.h" #endif TNetFile.cxx: #include "TNetFile.h" auto v = kROOTD_OPEN; TNetFile.h: #ifndef ROOT_TNetFile #define ROOT_TNetFile #include "MessageTypes.h" #endif https://reviews.llvm.org/D28832 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28832: Improve redefinition errors pointing to the same header.
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 = 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
[PATCH] D28832: Improve redefinition errors pointing to the same header.
bruno added a comment. Thanks Vassil. @rsmith is this ok for you? https://reviews.llvm.org/D28832 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28832: Improve redefinition errors pointing to the same header.
v.g.vassilev added a comment. This looks good to me. https://reviews.llvm.org/D28832 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28832: Improve redefinition errors pointing to the same header.
bruno created this revision. Diagnostics related to redefinition errors that point to the same header file do not provide much information that helps fixing the issue. In modules context it usually happens because of a non modular include, in non-module context it might happen because of the lack of header guardas. This patch tries to improve this situation by enhancing diagnostics in this particular scenario. If the approach seems reasonable, I can extend it to other relevant redefinition_error diagnostic call sites. Modules --- In file included from x.c:2: Inputs4/C.h:3:5: error: redefinition of 'c' int c = 1; ^ In module 'X' imported from x.c:1: Inputs4/C.h:3:5: note: previous definition is here int c = 1; ^ 1 warning and 1 error generated. After this patch In file included from x.c:2: Inputs4/C.h:3:5: error: redefinition of 'c' int c = 1; ^ In module 'X' imported from x.c:1: Inputs4/B.h:3:10: note: 'Inputs4/C.h' included multiple times, additional (likely non-modular) include site in module 'X.B' #include "C.h" ^ 1 error generated. Inputs4/module.modulemap:6:10: note: consider adding 'Inputs4/C.h' as part of 'X.B' definition in module B { ^ 1 error generated. Without Modules --- In file included from x.c:2: ./a.h:1:5: error: redefinition of 'yyy' int yyy = 42; ^ ./a.h:1:5: note: previous definition is here int yyy = 42; ^ 1 error generated. After this patch In file included from x.c:2: ./a.h:1:5: error: redefinition of 'yyy' int yyy = 42; ^ x.c:1:10: note: './a.h' included multiple times, consider augmenting this header with #ifdef guards #include "a.h" ^ 1 error generated. https://reviews.llvm.org/D28832 Files: include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaDecl.cpp test/Modules/Inputs/SameHeader/A.h test/Modules/Inputs/SameHeader/B.h test/Modules/Inputs/SameHeader/C.h test/Modules/Inputs/SameHeader/module.modulemap test/Modules/redefinition-same-header.m test/Sema/redefinition-same-header.c Index: test/Sema/redefinition-same-header.c === --- /dev/null +++ test/Sema/redefinition-same-header.c @@ -0,0 +1,12 @@ +// RUN: rm -rf %t +// RUN: mkdir -p %t +// RUN: echo 'int yyy = 42;' > %t/a.h +// RUN: %clang_cc1 -fsyntax-only %s -I%t -verify + +// expected-error@a.h:1 {{redefinition of 'yyy'}} +// expected-note-re@redefinition-same-header.c:9 {{'{{.*}}/a.h' included multiple times, consider augmenting this header with #ifdef guards}} + +#include "a.h" +#include "a.h" + +int foo() { return yyy; } Index: test/Modules/redefinition-same-header.m === --- /dev/null +++ test/Modules/redefinition-same-header.m @@ -0,0 +1,10 @@ +// RUN: rm -rf %t.tmp +// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SameHeader -fmodules \ +// RUN: -fimplicit-module-maps -fmodules-cache-path=%t.tmp %s -verify + +// expected-error@Inputs/SameHeader/C.h:3 {{redefinition of 'c'}} +// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional (likely non-modular) include site in module 'X.B'}} +// expected-note-re@Inputs/SameHeader/module.modulemap:6 {{consider adding '{{.*}}/C.h' as part of 'X.B' definition in}} + +#include "A.h" // maps to a modular +#include "C.h" // textual include Index: test/Modules/Inputs/SameHeader/module.modulemap === --- /dev/null +++ test/Modules/Inputs/SameHeader/module.modulemap @@ -0,0 +1,11 @@ +module X { + module A { +header "A.h" +export * + } + module B { +header "B.h" +export * + } + export * +} Index: test/Modules/Inputs/SameHeader/C.h === --- /dev/null +++ test/Modules/Inputs/SameHeader/C.h @@ -0,0 +1,4 @@ +#ifndef __C_h__ +#define __C_h__ +int c = 1; +#endif Index: test/Modules/Inputs/SameHeader/B.h === --- /dev/null +++ test/Modules/Inputs/SameHeader/B.h @@ -0,0 +1,4 @@ +#ifndef __B_h__ +#define __B_h__ +#include "C.h" +#endif Index: test/Modules/Inputs/SameHeader/A.h === --- /dev/null +++ test/Modules/Inputs/SameHeader/A.h @@ -0,0 +1,3 @@ +#ifndef __A_h__ +#define __A_h__ +#endif Index: lib/Sema/SemaDecl.cpp === --- lib/Sema/SemaDecl.cpp +++ lib/Sema/SemaDecl.cpp @@ -3728,6 +3728,49 @@ New->setImplicitlyInline(); } +void Sema::diagnoseRedefinition(SourceLocation Old, SourceLocation New) { + SourceManager = getSourceManager(); + auto FNewDecLoc = SrcMgr.getDecomposedLoc(New); + auto FOldDecLoc = SrcMgr.getDecomposedLoc(Old); + auto *FNew = SrcMgr.getFileEntryForID(FNewDecLoc.first); + auto *FOld = SrcMgr.getFileEntryForID(FOldDecLoc.first); + // Is it the