[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-04-11 Thread Iain Sandoe via Phabricator via cfe-commits 
iains marked an inline comment as done.
iains added inline comments.



Comment at: clang/lib/Sema/SemaModule.cpp:814-815
+diagExportedUnnamedDecl(S, UnnamedDeclKind::Namespace, D, BlockStart);
+  else
+; // We allow an empty named namespace decl.
+} else if (DC->getRedeclContext()->isFileContext() && !isa(D))

erichkeane wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > I think we should remove it. So that the above `if` could be further 
> > > merged.
> > we can remove the else ; but we cannot merge the !HasName into the if (that 
> > changes the logic of the expression such that empty & unnamed namespace 
> > decls get passed to checkExportedDeclContext()) - which emits a diagnostic 
> > not expected by the standard.
> > 
> > For me, the 'else ;' makes it clear what the alternative is doing (and 
> > there's a comment as well) - but if the general opinions is to remove it, 
> > that's also fine.
> I'd much prefer we did NOT do the empty else/semicolon here.  This ends up 
> causing a warning with GCC ( warning: suggest braces around empty body in an 
> ‘else’ statement [-Wempty-body]  ) and is jsut strange.  I don't mind a 
> comment, but the else/; is strange.
I committed:
rG92fed06f800a: [C++20][Modules] Remove an empty statement [NFC]. 
to resolve this.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-04-08 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaModule.cpp:814-815
+diagExportedUnnamedDecl(S, UnnamedDeclKind::Namespace, D, BlockStart);
+  else
+; // We allow an empty named namespace decl.
+} else if (DC->getRedeclContext()->isFileContext() && !isa(D))

iains wrote:
> ChuanqiXu wrote:
> > I think we should remove it. So that the above `if` could be further merged.
> we can remove the else ; but we cannot merge the !HasName into the if (that 
> changes the logic of the expression such that empty & unnamed namespace decls 
> get passed to checkExportedDeclContext()) - which emits a diagnostic not 
> expected by the standard.
> 
> For me, the 'else ;' makes it clear what the alternative is doing (and 
> there's a comment as well) - but if the general opinions is to remove it, 
> that's also fine.
I'd much prefer we did NOT do the empty else/semicolon here.  This ends up 
causing a warning with GCC ( warning: suggest braces around empty body in an 
‘else’ statement [-Wempty-body]  ) and is jsut strange.  I don't mind a 
comment, but the else/; is strange.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-04-08 Thread Iain Sandoe via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf60dc3caa673: [C++20][Modules] Adjust handling of exports of 
namespaces and using-decls. (authored by iains).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaModule.cpp
  clang/test/CXX/module/module.interface/p3.cpp
  clang/test/CXX/module/module.interface/p5.cpp
  clang/test/CXX/module/module.interface/p6.cpp
  clang/test/Modules/cxx20-10-2-ex1.cpp
  clang/test/Modules/cxx20-10-2-ex3.cpp
  clang/test/Modules/cxx20-10-2-ex4.cpp
  clang/test/Modules/cxx20-10-2-ex5.cpp
  clang/test/Modules/cxx20-10-2-ex6.cpp
  clang/test/Modules/cxx20-10-2-ex7.cpp

Index: clang/test/Modules/cxx20-10-2-ex7.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex7.cpp
@@ -0,0 +1,9 @@
+// Based on C++20 10.2 example 6.
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %s -verify -o M.pcm
+
+export module M;
+export namespace N {
+int x; // OK
+static_assert(1 == 1); // expected-error {{static_assert declaration cannot be exported}}
+} // namespace N
Index: clang/test/Modules/cxx20-10-2-ex6.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex6.cpp
@@ -0,0 +1,21 @@
+// Based on C++20 10.2 example 6.
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %s -verify -o M.pcm
+
+export module M;
+
+static int f();  // expected-note {{previous declaration is here}} #1
+ // error: #1 gives internal linkage
+export int f();  // expected-error {{cannot export redeclaration 'f' here since the previous declaration has internal linkage}}
+struct S;// expected-note {{previous declaration is here}} #2
+ // error: #2 gives module linkage
+export struct S; // expected-error {{cannot export redeclaration 'S' here since the previous declaration has module linkage}}
+
+namespace {
+namespace N {
+extern int x; // expected-note {{previous declaration is here}} #3
+}
+} // namespace
+  // error: #3 gives internal linkage
+export int N::x; // expected-error {{cannot export redeclaration 'x' here since the previous declaration has internal linkage}}
+ // expected-error@-1 {{declaration of 'x' with internal linkage cannot be exported}}
Index: clang/test/Modules/cxx20-10-2-ex5.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex5.cpp
@@ -0,0 +1,54 @@
+// Based on C++20 10.2 example 5.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/std-10-2-ex5-tu1.cpp \
+// RUN:  -o  %t/M.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj %t/std-10-2-ex5-tu2.cpp \
+// RUN:  -fmodule-file=%t/M.pcm -o  %t/tu-2.o
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj %t/std-10-2-ex5-tu3.cpp \
+// RUN:  -fmodule-file=%t/M.pcm -verify -o %t/main.o
+
+//--- std-10-2-ex5-tu1.cpp
+export module M;
+export struct X {
+  static void f();
+  struct Y {};
+};
+namespace {
+struct S {};
+} // namespace
+export void f(S); // OK
+struct T {};
+export T id(T);  // OK
+export struct A; // A exported as incomplete
+
+export auto rootFinder(double a) {
+  return [=](double x) { return (x + a / x) / 2; };
+}
+export const int n = 5; // OK, n has external linkage
+
+//--- std-10-2-ex5-tu2.cpp
+
+module M;
+struct A {
+  int value;
+};
+
+//--- std-10-2-ex5-tu3.cpp
+
+import M;
+
+int main() {
+  X::f(); // OK, X is exported and definition of X is reachable
+  X::Y y; // OK, X::Y is exported as a complete type
+  auto f = rootFinder(2); // OK
+  // error: A is incomplete
+  return A{45}.value; // expected-error {{invalid use of incomplete type 'A'}}
+  // expected-error@-1 {{member access into incomplete type 'A'}}
+  // expected-n...@std-10-2-ex5-tu1.cpp:12 2{{forward declaration of 'A'}}
+}
Index: clang/test/Modules/cxx20-10-2-ex4.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex4.cpp
@@ -0,0 +1,12 @@
+// Based on C++20 10.2 example 4.
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %s -verify -o M.pcm
+
+export module M;
+
+struct S { // expected-note {{previous declaration is here}}
+  int n;
+};
+typedef S S;
+export typedef S S; // OK, does not redeclare an entity
+export struct S;// expected-error {{cannot export redeclaration 'S' here since the previous declaration has module linkage}}
Index: clang/test/Modules/cxx20-10-2-ex3.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex3.cpp
@@ -0,0

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-04-06 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie accepted this revision.
dblaikie added a comment.

In D122119#3432220 , @iains wrote:

> @dblaikie - is the explanation for the change in diagnostics at the same time 
> OK? (if not, I am happy to split the patch, but either way I'd like to land 
> what is acceptable soonish).

I think it'll be OK as-is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-04-06 Thread Iain Sandoe via Phabricator via cfe-commits 
iains added a comment.

@dblaikie - is the explanation for the change in diagnostics at the same time 
OK? (if not I can split the patch, but either way I'd like to land what is 
acceptable soonish).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-24 Thread Iain Sandoe via Phabricator via cfe-commits 
iains added inline comments.



Comment at: clang/lib/Sema/SemaModule.cpp:814-815
+diagExportedUnnamedDecl(S, UnnamedDeclKind::Namespace, D, BlockStart);
+  else
+; // We allow an empty named namespace decl.
+} else if (DC->getRedeclContext()->isFileContext() && !isa(D))

ChuanqiXu wrote:
> I think we should remove it. So that the above `if` could be further merged.
we can remove the else ; but we cannot merge the !HasName into the if (that 
changes the logic of the expression such that empty & unnamed namespace decls 
get passed to checkExportedDeclContext()) - which emits a diagnostic not 
expected by the standard.

For me, the 'else ;' makes it clear what the alternative is doing (and there's 
a comment as well) - but if the general opinions is to remove it, that's also 
fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-24 Thread Iain Sandoe via Phabricator via cfe-commits 
iains updated this revision to Diff 418038.
iains marked 3 inline comments as done.
iains added a comment.

rebased, addressed review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaModule.cpp
  clang/test/CXX/module/module.interface/p3.cpp
  clang/test/CXX/module/module.interface/p5.cpp
  clang/test/CXX/module/module.interface/p6.cpp
  clang/test/Modules/cxx20-10-2-ex1.cpp
  clang/test/Modules/cxx20-10-2-ex3.cpp
  clang/test/Modules/cxx20-10-2-ex4.cpp
  clang/test/Modules/cxx20-10-2-ex5.cpp
  clang/test/Modules/cxx20-10-2-ex6.cpp
  clang/test/Modules/cxx20-10-2-ex7.cpp

Index: clang/test/Modules/cxx20-10-2-ex7.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex7.cpp
@@ -0,0 +1,9 @@
+// Based on C++20 10.2 example 6.
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %s -verify -o M.pcm
+
+export module M;
+export namespace N {
+int x; // OK
+static_assert(1 == 1); // expected-error {{static_assert declaration cannot be exported}}
+} // namespace N
Index: clang/test/Modules/cxx20-10-2-ex6.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex6.cpp
@@ -0,0 +1,21 @@
+// Based on C++20 10.2 example 6.
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %s -verify -o M.pcm
+
+export module M;
+
+static int f();  // expected-note {{previous declaration is here}} #1
+ // error: #1 gives internal linkage
+export int f();  // expected-error {{cannot export redeclaration 'f' here since the previous declaration has internal linkage}}
+struct S;// expected-note {{previous declaration is here}} #2
+ // error: #2 gives module linkage
+export struct S; // expected-error {{cannot export redeclaration 'S' here since the previous declaration has module linkage}}
+
+namespace {
+namespace N {
+extern int x; // expected-note {{previous declaration is here}} #3
+}
+} // namespace
+  // error: #3 gives internal linkage
+export int N::x; // expected-error {{cannot export redeclaration 'x' here since the previous declaration has internal linkage}}
+ // expected-error@-1 {{declaration of 'x' with internal linkage cannot be exported}}
Index: clang/test/Modules/cxx20-10-2-ex5.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex5.cpp
@@ -0,0 +1,54 @@
+// Based on C++20 10.2 example 5.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/std-10-2-ex5-tu1.cpp \
+// RUN:  -o  %t/M.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj %t/std-10-2-ex5-tu2.cpp \
+// RUN:  -fmodule-file=%t/M.pcm -o  %t/tu-2.o
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj %t/std-10-2-ex5-tu3.cpp \
+// RUN:  -fmodule-file=%t/M.pcm -verify -o %t/main.o
+
+//--- std-10-2-ex5-tu1.cpp
+export module M;
+export struct X {
+  static void f();
+  struct Y {};
+};
+namespace {
+struct S {};
+} // namespace
+export void f(S); // OK
+struct T {};
+export T id(T);  // OK
+export struct A; // A exported as incomplete
+
+export auto rootFinder(double a) {
+  return [=](double x) { return (x + a / x) / 2; };
+}
+export const int n = 5; // OK, n has external linkage
+
+//--- std-10-2-ex5-tu2.cpp
+
+module M;
+struct A {
+  int value;
+};
+
+//--- std-10-2-ex5-tu3.cpp
+
+import M;
+
+int main() {
+  X::f(); // OK, X is exported and definition of X is reachable
+  X::Y y; // OK, X::Y is exported as a complete type
+  auto f = rootFinder(2); // OK
+  // error: A is incomplete
+  return A{45}.value; // expected-error {{invalid use of incomplete type 'A'}}
+  // expected-error@-1 {{member access into incomplete type 'A'}}
+  // expected-n...@std-10-2-ex5-tu1.cpp:12 2{{forward declaration of 'A'}}
+}
Index: clang/test/Modules/cxx20-10-2-ex4.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex4.cpp
@@ -0,0 +1,12 @@
+// Based on C++20 10.2 example 4.
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %s -verify -o M.pcm
+
+export module M;
+
+struct S { // expected-note {{previous declaration is here}}
+  int n;
+};
+typedef S S;
+export typedef S S; // OK, does not redeclare an entity
+export struct S;// expected-error {{cannot export redeclaration 'S' here since the previous declaration has module linkage}}
Index: clang/test/Modules/cxx20-10-2-ex3.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex3.cpp
@@ -0,0 +1,9 @@
+// Based on C++20 10.2 example 3.
+
+// RUN:

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-24 Thread Iain Sandoe via Phabricator via cfe-commits 
iains added a comment.

In D122119#3400267 , @dblaikie wrote:

> SOrry, I don't have much context here - the more informative (module/internal 
> linkage) diagnostic does seem better to me than saying "is not exported", 
> even if it's a bit esoteric for some users. We do have other diagnostics that 
> mention linkage, I'm sure (because it's necessary/useful to describe certain 
> things).
>
> Without much context on this patch: is the diagnostic change a necessary part 
> of the patch? (is the diagnostic new regardless of which wording option is 
> chosen? or is this affecting an existing diagnostic?) - if it's possible to 
> separate the diagnostic change from the rest of the patch that's probably a 
> good thing to do regardless of the choice. If not, yeah, I think going with 
> the more explicit/nuanced diagnostic wording seems better to me at a glance.

The diagnostic change was made so that the test cases introduced by the patch 
could have (diagnostic) wording that reflected the examples in the standard.  
As such, it seemed reasonable to make the change at the same time.  I can split 
it out of course if that still seems better given the clarification.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-23 Thread Chuanqi Xu via Phabricator via cfe-commits 
ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

In D122119#3402168 , @urnathan wrote:

> In D122119#3401322 , @ChuanqiXu 
> wrote:
>
>> 
>
>
>
>> I think the example is invalid since it violates [[ 
>> http://eel.is/c++draft/module.interface#6 | [module.interface]p6 ]] 
>> explicitly. If this is intended behavior or by design, we should change the 
>> wording.
>
> That wording failed to be updated when the linkage-decl changes went in.  
> I'll add a note to DR 2541. Linkage specifications, module purview, and 
> module attachment

OK, if the wording is not consistent with the design, I have no further 
objections.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-23 Thread Nathan Sidwell via Phabricator via cfe-commits 
urnathan added a comment.

In D122119#3401322 , @ChuanqiXu wrote:

> 



> I think the example is invalid since it violates [[ 
> http://eel.is/c++draft/module.interface#6 | [module.interface]p6 ]] 
> explicitly. If this is intended behavior or by design, we should change the 
> wording.

That wording failed to be updated when the linkage-decl changes went in.  I'll 
add a note to DR 2541. Linkage specifications, module purview, and module 
attachment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-22 Thread Chuanqi Xu via Phabricator via cfe-commits 
ChuanqiXu added a comment.

In D122119#3400032 , @urnathan wrote:

> In D122119#3398949 , @ChuanqiXu 
> wrote:
>
>> 
>
>
>
>> The first feeling I saw the change is that not every C++ programmer knows 
>> about linkage. OK, it depends on the environment really and every one might 
>> has their own opinion.
>
> You may be confusing object-file linkage with the linkage concepts of C++, 
> which are specified in [basic.link]?  Sadly we have to live with the 
> overloaded term.
>
>> Another thought is that 10.2.6 (http://eel.is/c++draft/module.interface#6) 
>> doesn't talk anything about linkage:
>>
>>> A redeclaration of an entity X is implicitly exported if X was introduced 
>>> by an exported declaration; otherwise it shall not be exported.
>>
>> So it looks like confusing to talk about linkage this time. In my 
>> imagination, there might be a such situation:
>>
>> A programmer met the error when he tries to export a redeclaration which is 
>> internal linkage (maybe a simple const variable). Then the message told him 
>> the internal linkage is not allowed to re-export. Then he removes the const 
>> specifier. Now he meets the error again. It tells that we couldn't export 
>> redeclaration which is module linkage. I guess he would feel bad. Then he 
>> might try to export the first declaration to get passed. However, the 
>> `const` specifier is lost in the case. And in the current message, I guess 
>> he would add export to the first declaration directly after he reads the 
>> message.
>
> further, with attachment, the original error 'cannot export as previous was 
> not exported' is not correct in general.  Consider:
>
> module;
> // Pretend this is from #include, ok?
> void Foo ();
> module bob;
> extern "C++" export void Foo ();  // can export even though prior was not 
> exported

I think the example is invalid since it violates [[ 
http://eel.is/c++draft/module.interface#6 | [module.interface]p6 ]] explicitly. 
If this is intended behavior or by design, we should change the wording.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-22 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

SOrry, I don't have much context here - the more informative (module/internal 
linkage) diagnostic does seem better to me than saying "is not exported", even 
if it's a bit esoteric for some users. We do have other diagnostics that 
mention linkage, I'm sure (because it's necessary/useful to describe certain 
things).

Without much context on this patch: is the diagnostic change a necessary part 
of the patch? (is the diagnostic new regardless of which wording option is 
chosen? or is this affecting an existing diagnostic?) - if it's possible to 
separate the diagnostic change from the rest of the patch that's probably a 
good thing to do regardless of the choice. If not, yeah, I think going with the 
more explicit/nuanced diagnostic wording seems better to me at a glance.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-22 Thread Nathan Sidwell via Phabricator via cfe-commits 
urnathan added a comment.

In D122119#3398949 , @ChuanqiXu wrote:

> 



> The first feeling I saw the change is that not every C++ programmer knows 
> about linkage. OK, it depends on the environment really and every one might 
> has their own opinion.

You may be confusing object-file linkage with the linkage concepts of C++, 
which are specified in [basic.link]?  Sadly we have to live with the overloaded 
term.

> Another thought is that 10.2.6 (http://eel.is/c++draft/module.interface#6) 
> doesn't talk anything about linkage:
>
>> A redeclaration of an entity X is implicitly exported if X was introduced by 
>> an exported declaration; otherwise it shall not be exported.
>
> So it looks like confusing to talk about linkage this time. In my 
> imagination, there might be a such situation:
>
> A programmer met the error when he tries to export a redeclaration which is 
> internal linkage (maybe a simple const variable). Then the message told him 
> the internal linkage is not allowed to re-export. Then he removes the const 
> specifier. Now he meets the error again. It tells that we couldn't export 
> redeclaration which is module linkage. I guess he would feel bad. Then he 
> might try to export the first declaration to get passed. However, the `const` 
> specifier is lost in the case. And in the current message, I guess he would 
> add export to the first declaration directly after he reads the message.

further, with attachment, the original error 'cannot export as previous was not 
exported' is not correct in general.  Consider:

module;
// Pretend this is from #include, ok?
void Foo ();
module bob;
extern "C++" export void Foo ();  // can export even though prior was not 
exported


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-22 Thread Nathan Sidwell via Phabricator via cfe-commits 
urnathan added a comment.

In D122119#3398823 , @iains wrote:

> 



> With the change here we now get:
> cannot export redeclaration 'f' here since the previous declaration has 
> internal linkage
> cannot export redeclaration 'S' here since the previous declaration has 
> module linkage
>
> which seems maybe to be more helpful to the user in telling them why.
>
> I hope others can weigh in with an opinion here  .. @dblaikie @vsapsai  ?

Yes, this is more helpful -- tell the user what the compiler thinks it is, 
rather than what it thinks it is not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-22 Thread Iain Sandoe via Phabricator via cfe-commits 
iains marked 2 inline comments as done.
iains added a comment.

In D122119#3398949 , @ChuanqiXu wrote:

> In D122119#3398823 , @iains wrote:
>
>> So the adjustment to the error message is something I am 50/50 about (IMO it 
>> makes some messages more useful, but maybe not needed in others).
>>
>> Without the change we get 
>> "cannot export redeclaration 'xxx' here since the previous declaration is 
>> not exported"
>>
>> So, e.g in C++20 10.2 example 6. every case has the same error message 
>> (which was what prompted me to make the change).
>>
>> With the change here we now get:
>> cannot export redeclaration 'f' here since the previous declaration has 
>> internal linkage
>> cannot export redeclaration 'S' here since the previous declaration has 
>> module linkage
>>
>> which seems maybe to be more helpful to the user in telling them why.
>>
>> I hope others can weigh in with an opinion here  .. @dblaikie @vsapsai  ?
>
> I am OK to wait for opinions from others. Let me talk it a little bit more.
>
> The first feeling I saw the change is that not every C++ programmer knows 
> about linkage. OK, it depends on the environment really and every one might 
> has their own opinion.
>
> Another thought is that 10.2.6 (http://eel.is/c++draft/module.interface#6) 
> doesn't talk anything about linkage:
>
>> A redeclaration of an entity X is implicitly exported if X was introduced by 
>> an exported declaration; otherwise it shall not be exported.
>
> So it looks like confusing to talk about linkage this time. In my 
> imagination, there might be a such situation:
>
> A programmer met the error when he tries to export a redeclaration which is 
> internal linkage (maybe a simple const variable). Then the message told him 
> the internal linkage is not allowed to re-export. Then he removes the const 
> specifier. Now he meets the error again. It tells that we couldn't export 
> redeclaration which is module linkage. I guess he would feel bad. Then he 
> might try to export the first declaration to get passed. However, the `const` 
> specifier is lost in the case. And in the current message, I guess he would 
> add export to the first declaration directly after he reads the message.

yes, that seems like a reasonable counter argument, and perhaps talking about 
linkage in a user message is a bit "implementor speak".. 
...  as I said, I was kind of 50/50 about this - I'll wait 48h for any other 
comments and then remove the change if there are no votes in favour.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-22 Thread Chuanqi Xu via Phabricator via cfe-commits 
ChuanqiXu added a comment.

In D122119#3398823 , @iains wrote:

> So the adjustment to the error message is something I am 50/50 about (IMO it 
> makes some messages more useful, but maybe not needed in others).
>
> Without the change we get 
> "cannot export redeclaration 'xxx' here since the previous declaration is not 
> exported"
>
> So, e.g in C++20 10.2 example 6. every case has the same error message (which 
> was what prompted me to make the change).
>
> With the change here we now get:
> cannot export redeclaration 'f' here since the previous declaration has 
> internal linkage
> cannot export redeclaration 'S' here since the previous declaration has 
> module linkage
>
> which seems maybe to be more helpful to the user in telling them why.
>
> I hope others can weigh in with an opinion here  .. @dblaikie @vsapsai  ?

I am OK to wait for opinions from others. Let me talk it a little bit more.

The first feeling I saw the change is that not every C++ programmer knows about 
linkage. OK, it depends on the environment really and every one might has their 
own opinion.

Another thought is that 10.2.6 (http://eel.is/c++draft/module.interface#6) 
doesn't talk anything about linkage:

> A redeclaration of an entity X is implicitly exported if X was introduced by 
> an exported declaration; otherwise it shall not be exported.

So it looks like confusing to talk about linkage this time. In my imagination, 
there might be a such situation:

A programmer met the error when he tries to export a redeclaration which is 
internal linkage (maybe a simple const variable). Then the message told him the 
internal linkage is not allowed to re-export. Then he removes the const 
specifier. Now he meets the error again. It tells that we couldn't export 
redeclaration which is module linkage. I guess he would feel bad. Then he might 
try to export the first declaration to get passed. However, the `const` 
specifier is lost in the case. And in the current message, I guess he would add 
export to the first declaration directly after he reads the message.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-22 Thread Iain Sandoe via Phabricator via cfe-commits 
iains marked 2 inline comments as done.
iains added subscribers: vsapsai, dblaikie.
iains added a comment.

So the adjustment to the error message is something I am 50/50 about (IMO it 
makes some messages more useful, but maybe not needed in others).

Without the change we get 
"cannot export redeclaration 'xxx' here since the previous declaration is not 
exported"

So, e.g in C++20 10.2 example 6. every case has the same error message (which 
was what prompted me to make the change).

With the change here we now get:
cannot export redeclaration 'f' here since the previous declaration has 
internal linkage
cannot export redeclaration 'S' here since the previous declaration has module 
linkage

which seems maybe to be more helpful to the user in telling them why.

I hope others can weigh in with an opinion here  .. @dblaikie @vsapsai  ?




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7832-7833
 def err_redeclaration_non_exported : Error <
-  "cannot export redeclaration %0 here since the previous declaration is not "
-  "exported">;
+  "cannot export redeclaration %0 here since the previous declaration "
+  "%select{is not exported|has internal linkage|has module linkage}1">;
 def err_invalid_declarator_global_scope : Error<

ChuanqiXu wrote:
> I feel this change is not intended?
the change is intentional, but we should maybe decide if it is more or less 
helpful to the user.



Comment at: clang/lib/Sema/SemaDecl.cpp:1671-1677
+  auto Lk = Old->getFormalLinkage();
+  int S = 0;
+  if (Lk == Linkage::InternalLinkage)
+S = 1;
+  else if (Lk == Linkage::ModuleLinkage)
+S = 2;
+  Diag(New->getLocation(), diag::err_redeclaration_non_exported) << New << S;

ChuanqiXu wrote:
> It looks like that this change isn't reflected in the summary. Although this 
> is not bad, I feel it is not good. From the perspective of the user, I feel 
> the new message here is a little bit more confusing.
it is reflected in the summary:

"
This adjusts error messages for exports involving redeclarations in modules to
be more specific about the reason that the decl has been rejected.
"
Iet us discuss whether this is more/less useful outside of the code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-21 Thread Chuanqi Xu via Phabricator via cfe-commits 
ChuanqiXu added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7832-7833
 def err_redeclaration_non_exported : Error <
-  "cannot export redeclaration %0 here since the previous declaration is not "
-  "exported">;
+  "cannot export redeclaration %0 here since the previous declaration "
+  "%select{is not exported|has internal linkage|has module linkage}1">;
 def err_invalid_declarator_global_scope : Error<

I feel this change is not intended?



Comment at: clang/lib/Sema/SemaDecl.cpp:1671-1677
+  auto Lk = Old->getFormalLinkage();
+  int S = 0;
+  if (Lk == Linkage::InternalLinkage)
+S = 1;
+  else if (Lk == Linkage::ModuleLinkage)
+S = 2;
+  Diag(New->getLocation(), diag::err_redeclaration_non_exported) << New << S;

It looks like that this change isn't reflected in the summary. Although this is 
not bad, I feel it is not good. From the perspective of the user, I feel the 
new message here is a little bit more confusing.



Comment at: clang/lib/Sema/SemaModule.cpp:774-776
+  if (auto UDK = getUnnamedDeclKind(D)) {
 diagExportedUnnamedDecl(S, *UDK, D, BlockStart);
+  }

Unnecessary change?



Comment at: clang/lib/Sema/SemaModule.cpp:783
 // instead.
-if (ND->getDeclName() && ND->getFormalLinkage() == InternalLinkage) {
+HasName = !!ND->getDeclName();
+if (HasName && ND->getFormalLinkage() == InternalLinkage) {

nit: the general style in clang/llvm I see is to use (bool) conversion.



Comment at: clang/lib/Sema/SemaModule.cpp:814-815
+diagExportedUnnamedDecl(S, UnnamedDeclKind::Namespace, D, BlockStart);
+  else
+; // We allow an empty named namespace decl.
+} else if (DC->getRedeclContext()->isFileContext() && !isa(D))

I think we should remove it. So that the above `if` could be further merged.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122119/new/

https://reviews.llvm.org/D122119

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

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

2022-03-21 Thread Iain Sandoe via Phabricator via cfe-commits 
iains created this revision.
Herald added a project: All.
iains added reviewers: rsmith, urnathan, ChuanqiXu.
iains added a subscriber: clang-modules.
iains published this revision for review.
iains added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

this covers most of the tests from the standard section 10.3 (one to follow 
that has an additional dependency on header units)

note the change in the "cannot export redeclaration ' here since the 
previous declaration" errors to include a more specific reason than before.


This adjusts the handling for:

export module  M;

export namespace {};

export namespace N {};
export using namespace N;

In the first case, we were allowing empty anonymous namespaces
as part of an extension allowing empty top-level entities, but that seems
inappropriate in this case, since the linkage would be internal for the
anonymous namespace.  We now report an error for this.

The second case was producing a warning diagnostic that this was
accepted as an extension - however the C++20 standard does allow this
as well-formed.

In the third case we keep the current practice that this is accepted with a
warning (as an extension). The C++20 standard says it's an error.

We also ensure that using decls are only applied to items with external linkage.

This adjusts error messages for exports involving redeclarations in modules to
be more specific about the reason that the decl has been rejected.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122119

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaModule.cpp
  clang/test/CXX/module/module.interface/p3.cpp
  clang/test/CXX/module/module.interface/p5.cpp
  clang/test/CXX/module/module.interface/p6.cpp
  clang/test/Modules/cxx20-10-2-ex1.cpp
  clang/test/Modules/cxx20-10-2-ex3.cpp
  clang/test/Modules/cxx20-10-2-ex4.cpp
  clang/test/Modules/cxx20-10-2-ex5.cpp
  clang/test/Modules/cxx20-10-2-ex6.cpp
  clang/test/Modules/cxx20-10-2-ex7.cpp

Index: clang/test/Modules/cxx20-10-2-ex7.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex7.cpp
@@ -0,0 +1,9 @@
+// Based on C++20 10.2 example 6.
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %s -verify -o M.pcm
+
+export module M;
+export namespace N {
+int x; // OK
+static_assert(1 == 1); // expected-error {{static_assert declaration cannot be exported}}
+} // namespace N
Index: clang/test/Modules/cxx20-10-2-ex6.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex6.cpp
@@ -0,0 +1,21 @@
+// Based on C++20 10.2 example 6.
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %s -verify -o M.pcm
+
+export module M;
+
+static int f();  // expected-note {{previous declaration is here}} #1
+ // error: #1 gives internal linkage
+export int f();  // expected-error {{cannot export redeclaration 'f' here since the previous declaration has internal linkage}}
+struct S;// expected-note {{previous declaration is here}} #2
+ // error: #2 gives module linkage
+export struct S; // expected-error {{cannot export redeclaration 'S' here since the previous declaration has module linkage}}
+
+namespace {
+namespace N {
+extern int x; // expected-note {{previous declaration is here}} #3
+}
+} // namespace
+  // error: #3 gives internal linkage
+export int N::x; // expected-error {{cannot export redeclaration 'x' here since the previous declaration has internal linkage}}
+ // expected-error@-1 {{declaration of 'x' with internal linkage cannot be exported}}
Index: clang/test/Modules/cxx20-10-2-ex5.cpp
===
--- /dev/null
+++ clang/test/Modules/cxx20-10-2-ex5.cpp
@@ -0,0 +1,54 @@
+// Based on C++20 10.2 example 5.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/std-10-2-ex5-tu1.cpp \
+// RUN:  -o  %t/M.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj %t/std-10-2-ex5-tu2.cpp \
+// RUN:  -fmodule-file=%t/M.pcm -o  %t/tu-2.o
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj %t/std-10-2-ex5-tu3.cpp \
+// RUN:  -fmodule-file=%t/M.pcm -verify -o %t/main.o
+
+//--- std-10-2-ex5-tu1.cpp
+export module M;
+export struct X {
+  static void f();
+  struct Y {};
+};
+namespace {
+struct S {};
+} // namespace
+export void f(S); // OK
+struct T {};
+export T id(T);  // OK
+export struct A; // A exported as incomplete
+
+export auto rootFinder(double a) {
+  return [=](double x) { return (x + a / x) / 2; };
+}
+export const int n = 5; // OK, n has external linkage
+
+//--- std-10-2-ex5-tu2.cpp
+
+module M;
+struct A {
+  int value;
+};
+
+//--- std-10-2-ex5-tu3.cpp
+
+import M;
+
+int main() {
+  X::f(); // OK, X is exported and definition of X is