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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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.

[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



___
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 Cardoso Lopes via Phabricator via cfe-commits]

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.

[Vassil Vassilev via Phabricator via cfe-commits]

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 Cardoso Lopes via Phabricator via cfe-commits]

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 Cardoso Lopes via Phabricator via cfe-commits]

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 Cardoso Lopes via Phabricator via cfe-commits]

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.

[Vassil Vassilev via Phabricator via cfe-commits]

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.

[Richard Smith via Phabricator via cfe-commits]

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 Cardoso Lopes via Phabricator via cfe-commits]

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.

[Vassil Vassilev via Phabricator via cfe-commits]

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 Cardoso Lopes via Phabricator via cfe-commits]

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