[PATCH] D34510: Teach clang how to merge typedef over anonymous structs in C mode.

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev closed this revision.
v.g.vassilev added a comment.

Landed in r306964.


Repository:
  rL LLVM

https://reviews.llvm.org/D34510



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34510: Teach clang how to merge typedef over anonymous structs in C mode.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

Tested it locally, we actually need all these changes on top of 
https://reviews.llvm.org/D31778.

LGTM


Repository:
  rL LLVM

https://reviews.llvm.org/D34510



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34510: Teach clang how to merge typedef over anonymous structs in C mode.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi @v.g.vassilev, sorry for the delay.

Just updated https://reviews.llvm.org/D31778. I agree with Richard's 
observations, do you think you can extend it to work in the same way as 
https://reviews.llvm.org/D31778 does? The structural checking is already 
abstracted there (should be straightforward to use).


Repository:
  rL LLVM

https://reviews.llvm.org/D34510



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34510: Teach clang how to merge typedef over anonymous structs in C mode.

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added a comment.

@bruno ping...


Repository:
  rL LLVM

https://reviews.llvm.org/D34510



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34510: Teach clang how to merge typedef over anonymous structs in C mode.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

https://reviews.llvm.org/D31778 is related. We need to have a design for how 
ODR-like issues in C will be resolved before we'll know what the right fix is 
here. Prior to https://reviews.llvm.org/D31778 my intention had been to 
implement C's structural typing ("compatible type") rules and keep multiple 
definitions of `RecordDecl`s in C as separate entities, but 
https://reviews.llvm.org/D31778 is suggesting that we use something more 
ODR-like instead, which is also more in line with the direction that this patch 
takes.

It's notable that C has no notion of "typedef name for linkage", so this patch 
does not address all the typedef-related issues that might show up in C code. 
For example:

  typedef struct { ... } *ptr;

in two different modules would not be merged into a single type with this 
approach (you'd need to actually implement the C compatible type rules to get 
that to work). But maybe that's OK -- the above also does not (formally) work 
in C++, and it doesn't seem to cause major problems.


Repository:
  rL LLVM

https://reviews.llvm.org/D34510



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34510: Teach clang how to merge typedef over anonymous structs in C mode.

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev created this revision.

empty.h makes module A available. textual.h brings the definition of __fsid_t 
and a.h imports again the definition of __fsid_t.

In C mode clang fails to merge the textually included definition with the one 
imported from a module. The C lookup rules fail to find the imported definition 
because its linkage is internal in non C++ mode.

This patch reinstates some of the ODR merging rules for typedefs of anonymous 
tags for languages other than C++.

Patch by Raphael Isemann and I.


Repository:
  rL LLVM

https://reviews.llvm.org/D34510

Files:
  lib/AST/Decl.cpp
  lib/Sema/SemaDecl.cpp
  test/Modules/Inputs/merge-typedefs-c/a.h
  test/Modules/Inputs/merge-typedefs-c/empty.h
  test/Modules/Inputs/merge-typedefs-c/module.modulemap
  test/Modules/Inputs/merge-typedefs-c/textual.h
  test/Modules/merge-typedefs-c.c


Index: test/Modules/merge-typedefs-c.c
===
--- /dev/null
+++ test/Modules/merge-typedefs-c.c
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -xc -fmodules-cache-path=%t 
-I%S/Inputs/merge-typedefs-c/ -fimplicit-module-maps -verify %s
+// RUN: %clang_cc1 -fmodules -xc++ -fmodules-cache-path=%t 
-I%S/Inputs/merge-typedefs-c/ -fimplicit-module-maps -verify %s
+
+// expected-no-diagnostics
+#include "empty.h"
+#include "textual.h"
+#include "a.h"
+
+__fsid_t use;
Index: test/Modules/Inputs/merge-typedefs-c/textual.h
===
--- /dev/null
+++ test/Modules/Inputs/merge-typedefs-c/textual.h
@@ -0,0 +1,7 @@
+#ifndef TEXTUAL_H
+#define TEXTUAL_H
+
+typedef struct { } __fsid_t;
+
+#endif
+
Index: test/Modules/Inputs/merge-typedefs-c/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/merge-typedefs-c/module.modulemap
@@ -0,0 +1,11 @@
+module "A" {
+   export *
+   module "a.h" {
+ export *
+ header "a.h"
+   }
+   module "empty.h" {
+ export *
+ header "empty.h"
+   }
+ }
Index: test/Modules/Inputs/merge-typedefs-c/empty.h
===
--- /dev/null
+++ test/Modules/Inputs/merge-typedefs-c/empty.h
@@ -0,0 +1 @@
+// This is a module trigger.
Index: test/Modules/Inputs/merge-typedefs-c/a.h
===
--- /dev/null
+++ test/Modules/Inputs/merge-typedefs-c/a.h
@@ -0,0 +1,4 @@
+#ifndef A_H
+#define A_H
+#include "textual.h"
+#endif
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -1996,8 +1996,7 @@
 
   // If both declarations give a tag declaration a typedef name for linkage
   // purposes, then they declare the same entity.
-  if (S.getLangOpts().CPlusPlus &&
-  OldTD->getAnonDeclWithTypedefName(/*AnyRedecl*/true) &&
+  if (OldTD->getAnonDeclWithTypedefName(/*AnyRedecl*/true) &&
   Decl->getAnonDeclWithTypedefName())
 continue;
 }
@@ -2115,7 +2114,7 @@
 auto *OldTag = OldTD->getAnonDeclWithTypedefName(/*AnyRedecl*/true);
 auto *NewTag = New->getAnonDeclWithTypedefName();
 NamedDecl *Hidden = nullptr;
-if (getLangOpts().CPlusPlus && OldTag && NewTag &&
+if (OldTag && NewTag &&
 OldTag->getCanonicalDecl() != NewTag->getCanonicalDecl() &&
 !hasVisibleDefinition(OldTag, )) {
   // There is a definition of this tag, but it is not visible. Use it
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -1257,8 +1257,7 @@
 case Decl::TypeAlias:
   // A typedef declaration has linkage if it gives a type a name for
   // linkage purposes.
-  if (!D->getASTContext().getLangOpts().CPlusPlus ||
-  !cast(D)
+  if (!cast(D)
->getAnonDeclWithTypedefName(/*AnyRedecl*/true))
 return LinkageInfo::none();
   break;


Index: test/Modules/merge-typedefs-c.c
===
--- /dev/null
+++ test/Modules/merge-typedefs-c.c
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -xc -fmodules-cache-path=%t -I%S/Inputs/merge-typedefs-c/ -fimplicit-module-maps -verify %s
+// RUN: %clang_cc1 -fmodules -xc++ -fmodules-cache-path=%t -I%S/Inputs/merge-typedefs-c/ -fimplicit-module-maps -verify %s
+
+// expected-no-diagnostics
+#include "empty.h"
+#include "textual.h"
+#include "a.h"
+
+__fsid_t use;
Index: test/Modules/Inputs/merge-typedefs-c/textual.h
===
--- /dev/null
+++ test/Modules/Inputs/merge-typedefs-c/textual.h
@@ -0,0 +1,7 @@
+#ifndef TEXTUAL_H
+#define TEXTUAL_H
+
+typedef struct { } __fsid_t;
+
+#endif
+
Index: test/Modules/Inputs/merge-typedefs-c/module.modulemap