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&id=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'">;
@@ -8888,6 +8890,13 @@
InGroup<DiagGroup<"modules-ambiguous-internal-linkage">>;
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: cfe/trunk/test/Modules/Inputs/SameHeader/C.h
===================================================================
--- cfe/trunk/test/Modules/Inputs/SameHeader/C.h
+++ cfe/trunk/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: cfe/trunk/test/Modules/Inputs/SameHeader/module.modulemap
===================================================================
--- cfe/trunk/test/Modules/Inputs/SameHeader/module.modulemap
+++ cfe/trunk/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: cfe/trunk/test/Modules/Inputs/SameHeader/B.h
===================================================================
--- cfe/trunk/test/Modules/Inputs/SameHeader/B.h
+++ cfe/trunk/test/Modules/Inputs/SameHeader/B.h
@@ -0,0 +1,4 @@
+#ifndef __B_h__
+#define __B_h__
+#include "C.h"
+#endif
Index: cfe/trunk/test/Modules/Inputs/SameHeader/A.h
===================================================================
--- cfe/trunk/test/Modules/Inputs/SameHeader/A.h
+++ cfe/trunk/test/Modules/Inputs/SameHeader/A.h
@@ -0,0 +1,3 @@
+#ifndef __A_h__
+#define __A_h__
+#endif
Index: cfe/trunk/test/Sema/redefinition-same-header.c
===================================================================
--- cfe/trunk/test/Sema/redefinition-same-header.c
+++ cfe/trunk/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: cfe/trunk/lib/Sema/SemaDecl.cpp
===================================================================
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -2021,7 +2021,7 @@
Diag(New->getLocation(), diag::err_redefinition_variably_modified_typedef)
<< Kind << NewType;
if (Old->getLocation().isValid())
- Diag(Old->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(Old->getLocation(), New->getLocation());
New->setInvalidDecl();
return true;
}
@@ -2034,7 +2034,7 @@
Diag(New->getLocation(), diag::err_redefinition_different_typedef)
<< Kind << NewType << OldType;
if (Old->getLocation().isValid())
- Diag(Old->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(Old->getLocation(), New->getLocation());
New->setInvalidDecl();
return true;
}
@@ -2101,7 +2101,7 @@
NamedDecl *OldD = OldDecls.getRepresentativeDecl();
if (OldD->getLocation().isValid())
- Diag(OldD->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(OldD->getLocation(), New->getLocation());
return New->setInvalidDecl();
}
@@ -2193,7 +2193,7 @@
Diag(New->getLocation(), diag::err_redefinition)
<< New->getDeclName();
- Diag(Old->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(Old->getLocation(), New->getLocation());
return New->setInvalidDecl();
}
@@ -2214,7 +2214,7 @@
Diag(New->getLocation(), diag::ext_redefinition_of_typedef)
<< New->getDeclName();
- Diag(Old->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(Old->getLocation(), New->getLocation());
}
/// DeclhasAttr - returns true if decl Declaration already has the target
@@ -2501,7 +2501,10 @@
? diag::err_alias_after_tentative
: diag::err_redefinition;
S.Diag(VD->getLocation(), Diag) << VD->getDeclName();
- S.Diag(Def->getLocation(), diag::note_previous_definition);
+ if (Diag == diag::err_redefinition)
+ S.notePreviousDefinition(Def->getLocation(), VD->getLocation());
+ else
+ S.Diag(Def->getLocation(), diag::note_previous_definition);
VD->setInvalidDecl();
}
++I;
@@ -2888,7 +2891,7 @@
} else {
Diag(New->getLocation(), diag::err_redefinition_different_kind)
<< New->getDeclName();
- Diag(OldD->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(OldD->getLocation(), New->getLocation());
return true;
}
}
@@ -2925,7 +2928,7 @@
!Old->hasAttr<InternalLinkageAttr>()) {
Diag(New->getLocation(), diag::err_internal_linkage_redeclaration)
<< New->getDeclName();
- Diag(Old->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(Old->getLocation(), New->getLocation());
New->dropAttr<InternalLinkageAttr>();
}
@@ -3653,9 +3656,9 @@
}
if (!Old) {
Diag(New->getLocation(), diag::err_redefinition_different_kind)
- << New->getDeclName();
- Diag(Previous.getRepresentativeDecl()->getLocation(),
- diag::note_previous_definition);
+ << New->getDeclName();
+ notePreviousDefinition(Previous.getRepresentativeDecl()->getLocation(),
+ New->getLocation());
return New->setInvalidDecl();
}
@@ -3684,16 +3687,16 @@
Old->getStorageClass() == SC_None &&
!Old->hasAttr<WeakImportAttr>()) {
Diag(New->getLocation(), diag::warn_weak_import) << New->getDeclName();
- Diag(Old->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(Old->getLocation(), New->getLocation());
// Remove weak_import attribute on new declaration.
New->dropAttr<WeakImportAttr>();
}
if (New->hasAttr<InternalLinkageAttr>() &&
!Old->hasAttr<InternalLinkageAttr>()) {
Diag(New->getLocation(), diag::err_internal_linkage_redeclaration)
<< New->getDeclName();
- Diag(Old->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(Old->getLocation(), New->getLocation());
New->dropAttr<InternalLinkageAttr>();
}
@@ -3850,6 +3853,67 @@
New->setImplicitlyInline();
}
+void Sema::notePreviousDefinition(SourceLocation Old, SourceLocation New) {
+ SourceManager &SrcMgr = getSourceManager();
+ auto FNewDecLoc = SrcMgr.getDecomposedLoc(New);
+ auto FOldDecLoc = SrcMgr.getDecomposedLoc(Old);
+ auto *FNew = SrcMgr.getFileEntryForID(FNewDecLoc.first);
+ auto *FOld = SrcMgr.getFileEntryForID(FOldDecLoc.first);
+ auto &HSI = PP.getHeaderSearchInfo();
+ StringRef HdrFilename = SrcMgr.getFilename(SrcMgr.getSpellingLoc(Old));
+
+ auto noteFromModuleOrInclude = [&](SourceLocation &Loc,
+ SourceLocation &IncLoc) -> bool {
+ Module *Mod = nullptr;
+ // Redefinition errors with modules are common with non modular mapped
+ // headers, example: a non-modular header H in module A that also gets
+ // included directly in a TU. Pointing twice to the same header/definition
+ // is confusing, try to get better diagnostics when modules is on.
+ if (getLangOpts().Modules) {
+ auto ModLoc = SrcMgr.getModuleImportLoc(Old);
+ if (!ModLoc.first.isInvalid())
+ Mod = HSI.getModuleMap().inferModuleFromLocation(
+ FullSourceLoc(Loc, SrcMgr));
+ }
+
+ if (IncLoc.isValid()) {
+ if (Mod) {
+ Diag(IncLoc, diag::note_redefinition_modules_same_file)
+ << HdrFilename.str() << Mod->getFullModuleName();
+ if (!Mod->DefinitionLoc.isInvalid())
+ Diag(Mod->DefinitionLoc, diag::note_defined_here)
+ << Mod->getFullModuleName();
+ } else {
+ Diag(IncLoc, diag::note_redefinition_include_same_file)
+ << HdrFilename.str();
+ }
+ return true;
+ }
+
+ return false;
+ };
+
+ // Is it the same file and same offset? Provide more information on why
+ // this leads to a redefinition error.
+ bool EmittedDiag = false;
+ if (FNew == FOld && FNewDecLoc.second == FOldDecLoc.second) {
+ SourceLocation OldIncLoc = SrcMgr.getIncludeLoc(FOldDecLoc.first);
+ SourceLocation NewIncLoc = SrcMgr.getIncludeLoc(FNewDecLoc.first);
+ EmittedDiag = noteFromModuleOrInclude(Old, OldIncLoc);
+ EmittedDiag |= noteFromModuleOrInclude(New, NewIncLoc);
+
+ // If the header has no guards, emit a note suggesting one.
+ if (FOld && !HSI.isFileMultipleIncludeGuarded(FOld))
+ Diag(Old, diag::note_use_ifdef_guards);
+
+ if (EmittedDiag)
+ return;
+ }
+
+ // Redefinition coming from different files or couldn't do better above.
+ Diag(Old, diag::note_previous_definition);
+}
+
/// We've just determined that \p Old and \p New both appear to be definitions
/// of the same variable. Either diagnose or fix the problem.
bool Sema::checkVarDeclRedefinition(VarDecl *Old, VarDecl *New) {
@@ -3870,7 +3934,7 @@
return false;
} else {
Diag(New->getLocation(), diag::err_redefinition) << New;
- Diag(Old->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(Old->getLocation(), New->getLocation());
New->setInvalidDecl();
return true;
}
@@ -13485,7 +13549,8 @@
Diag(NameLoc, diag::warn_redefinition_in_param_list) << Name;
else
Diag(NameLoc, diag::err_redefinition) << Name;
- Diag(Def->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(Def->getLocation(),
+ NameLoc.isValid() ? NameLoc : KWLoc);
// If this is a redefinition, recover by making this
// struct be anonymous, which will make any later
// references get the previous definition.
@@ -13575,7 +13640,7 @@
// The tag name clashes with something else in the target scope,
// issue an error and recover by making this tag be anonymous.
Diag(NameLoc, diag::err_redefinition_different_kind) << Name;
- Diag(PrevDecl->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(PrevDecl->getLocation(), NameLoc);
Name = nullptr;
Invalid = true;
}
@@ -15279,7 +15344,7 @@
Diag(IdLoc, diag::err_redefinition_of_enumerator) << Id;
else
Diag(IdLoc, diag::err_redefinition) << Id;
- Diag(PrevDecl->getLocation(), diag::note_previous_definition);
+ notePreviousDefinition(PrevDecl->getLocation(), IdLoc);
return nullptr;
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
