[PATCH] D146986: Downgrade reserved module identifier error into a warning

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:92
+  `_ and allows easier
+  building of precompiled modules. This diagnostic may be strengthened into an
+  error again in the future once there is a less fragile way to mark a module

Mordante wrote:
> Note this is about the standard modules, which are not precompiled per se.
Thanks! I fixed this up on the Clang 17 branch in 
711b70d77e99dd4101b9dfa94d9ea0d4149997ab


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146986

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

[PATCH] D146986: Downgrade reserved module identifier error into a warning

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

Sorry I was not available earlier. Thanks for working on this. LGTM but one 
remark.




Comment at: clang/docs/ReleaseNotes.rst:92
+  `_ and allows easier
+  building of precompiled modules. This diagnostic may be strengthened into an
+  error again in the future once there is a less fragile way to mark a module

Note this is about the standard modules, which are not precompiled per se.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146986

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

[PATCH] D146986: Downgrade reserved module identifier error into a warning

[Aaron Ballman via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe574833c2bc4: Downgrade reserved module identifier error 
into a warning (authored by aaron.ballman).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146986

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaModule.cpp
  clang/test/Modules/reserved-names-1.cpp

Index: clang/test/Modules/reserved-names-1.cpp
===
--- clang/test/Modules/reserved-names-1.cpp
+++ clang/test/Modules/reserved-names-1.cpp
@@ -1,34 +1,35 @@
-// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %s
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %s
 
 // expected-note@1 15{{add 'module;' to the start of the file to introduce a global module fragment}}
 
-module std;// expected-error {{'std' is a reserved name for a module}}
-module _Test;  // expected-error {{'_Test' is a reserved name for a module}} \
+module std;// loud-warning {{'std' is a reserved name for a module}}
+module _Test;  // loud-warning {{'_Test' is a reserved name for a module}} \
   expected-error {{module declaration must occur at the start of the translation unit}}
 module module; // expected-error {{'module' is an invalid name for a module}} \
   expected-error {{module declaration must occur at the start of the translation unit}}
-module std0;   // expected-error {{'std0' is a reserved name for a module}} \
+module std0;   // loud-warning {{'std0' is a reserved name for a module}} \
   expected-error {{module declaration must occur at the start of the translation unit}}
 
 export module module; // expected-error {{'module' is an invalid name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
 export module import; // expected-error {{'import' is an invalid name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module _Test;  // expected-error {{'_Test' is a reserved name for a module}} \
+export module _Test;  // loud-warning {{'_Test' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module __test; // expected-error {{'__test' is a reserved name for a module}} \
+export module __test; // loud-warning {{'__test' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module te__st; // expected-error {{'te__st' is a reserved name for a module}} \
+export module te__st; // loud-warning {{'te__st' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module std;// expected-error {{'std' is a reserved name for a module}} \
+export module std;// loud-warning {{'std' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module std.foo;// expected-error {{'std' is a reserved name for a module}} \
+export module std.foo;// loud-warning {{'std' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module std0;   // expected-error {{'std0' is a reserved name for a module}} \
+export module std0;   // loud-warning {{'std0' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module std100; // expected-error {{'std100' is a reserved name for a module}} \
+export module std100; // loud-warning {{'std100' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module should_fail._Test; // expected-error {{'_Test' is a reserved name for a module}} \
+export module should_diag._Test; // loud-warning {{'_Test' is a reserved name for a module}} \
 expected-error {{module declaration must occur at the start of the translation unit}}
 
 // Show that being in a system header doesn't save you from diagnostics about
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -156,11 +156,15 @@
   if (Reason == Reserved && 

[PATCH] D146986: Downgrade reserved module identifier error into a warning

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

+1 for this as an interim solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146986

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

[PATCH] D146986: Downgrade reserved module identifier error into a warning

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D146986#4225275 , @dblaikie wrote:

> Presumably adding an alias for `#pragma clang system_header` called 
> `system_module` wouldn't be too hard? (though the pragma is also being 
> removed from libc++ soon, I think, in favor of `-isystem` usage, so maybe 
> that's a sign the pragma's not a great way to do)

It wouldn't be hard to add, but the modules group wanted to see if we can find 
some common feature that all the implementations implement instead of relying 
on different extensions for different compilers, IIUC. So that could be a 
pragma, or it could be an attribute attached to the module name, etc.

> Presumably the line marker issue would be less significant for a module? 
> Since there's no complex line marking, inclusion, etc, going on - it's just 
> where the actual .cppm file is located/how it's found? (though yeah, that 
> might get weird for building .pcms - since you're going to name the source 
> file directly on the command line, it's not going to be found via any isystem 
> lookup, etc... )

The trouble is maintenance. Someone ran into a problem where they added code to 
the module and then diagnostics started showing up on the wrong lines because 
of the line marker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146986

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

[PATCH] D146986: Downgrade reserved module identifier error into a warning

[Chuanqi Xu via Phabricator via cfe-commits]

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

> Presumably adding an alias for #pragma clang system_header called 
> system_module wouldn't be too hard? (though the pragma is also being removed 
> from libc++ soon, I think, in favor of -isystem usage, so maybe that's a sign 
> the pragma's not a great way to do)

Yeah, it shouldn't be hard but it looks not good too. I feel the current patch 
is better/simpler as a workaround.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146986

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

[PATCH] D146986: Downgrade reserved module identifier error into a warning

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D146986#4225192 , @aaron.ballman 
wrote:

> In D146986#4225121 , @dblaikie 
> wrote:
>
>> From the discussion on the issue:
>>
>>> Do we want this loosening of the restriction to apply to *only* `std` and 
>>> the same followed by a number, or to any reserved identifier used in a 
>>> module? e.g.,
>>>
>>>   module std; // error today, but will become a warning
>>>   module _Test; // error today, but do we want this to become a warning as 
>>> well?
>>>
>>> my thinking is we probably want all of these to be warnings because it'd be 
>>> hard to explain why `std` is reserved but with a warning while `_Test` is 
>>> reserved but with an error.
>>
>> Yeah, I'd treat them equally - while we could subset the reserved names and 
>> allow implementations to only use a subset (while leaving the rest as an 
>> error for both implementations and consumers alike) that doesn't feel in 
>> keeping with the purpose of these names - to be usable by /someone/ and so 
>> necessary to allow them to be used.
>>
>> (hmm - there's some discussion in the description about the fact that this 
>> error was already suppressed in "system headers" - why was that suppression 
>> inadequate for system implementation modules? (& does that suppression for 
>> reserved names risk being over-broad, since every third party library 
>> installed on a system is generally considered a "system header", even if 
>> they aren't part of the implementation?))
>
> We currently use line markers to "enter" a system header and that's quite 
> fragile. I mentioned we could use `#pragma clang system_header`, but 
> @ChuanqiXu  didn't think that was appropriate because these are not headers, 
> they're modules, and we should have some separation between "system headers" 
> and "system modules". 
> (https://github.com/llvm/llvm-project/issues/61446#issuecomment-1473029776) 
> As for being over-broad, it might be, but this is the approach we usually 
> take (anything that's a "system header" is considered special and gets less 
> diagnostics because the user isn't typically able to change the contents of 
> the header file anyway).

Presumably adding an alias for `#pragma clang system_header` called 
`system_module` wouldn't be too hard? (though the pragma is also being removed 
from libc++ soon, I think, in favor of `-isystem` usage, so maybe that's a sign 
the pragma's not a great way to do)

Presumably the line marker issue would be less significant for a module? Since 
there's no complex line marking, inclusion, etc, going on - it's just where the 
actual .cppm file is located/how it's found? (though yeah, that might get weird 
for building .pcms - since you're going to name the source file directly on the 
command line, it's not going to be found via any isystem lookup, etc... )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146986

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

[PATCH] D146986: Downgrade reserved module identifier error into a warning

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D146986#4225121 , @dblaikie wrote:

> From the discussion on the issue:
>
>> Do we want this loosening of the restriction to apply to *only* `std` and 
>> the same followed by a number, or to any reserved identifier used in a 
>> module? e.g.,
>>
>>   module std; // error today, but will become a warning
>>   module _Test; // error today, but do we want this to become a warning as 
>> well?
>>
>> my thinking is we probably want all of these to be warnings because it'd be 
>> hard to explain why `std` is reserved but with a warning while `_Test` is 
>> reserved but with an error.
>
> Yeah, I'd treat them equally - while we could subset the reserved names and 
> allow implementations to only use a subset (while leaving the rest as an 
> error for both implementations and consumers alike) that doesn't feel in 
> keeping with the purpose of these names - to be usable by /someone/ and so 
> necessary to allow them to be used.
>
> (hmm - there's some discussion in the description about the fact that this 
> error was already suppressed in "system headers" - why was that suppression 
> inadequate for system implementation modules? (& does that suppression for 
> reserved names risk being over-broad, since every third party library 
> installed on a system is generally considered a "system header", even if they 
> aren't part of the implementation?))

We currently use line markers to "enter" a system header and that's quite 
fragile. I mentioned we could use `#pragma clang system_header`, but @ChuanqiXu 
 didn't think that was appropriate because these are not headers, they're 
modules, and we should have some separation between "system headers" and 
"system modules". 
(https://github.com/llvm/llvm-project/issues/61446#issuecomment-1473029776) As 
for being over-broad, it might be, but this is the approach we usually take 
(anything that's a "system header" is considered special and gets less 
diagnostics because the user isn't typically able to change the contents of the 
header file anyway).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146986

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

[PATCH] D146986: Downgrade reserved module identifier error into a warning

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

From the discussion on the issue:

> Do we want this loosening of the restriction to apply to *only* `std` and the 
> same followed by a number, or to any reserved identifier used in a module? 
> e.g.,
>
>   module std; // error today, but will become a warning
>   module _Test; // error today, but do we want this to become a warning as 
> well?
>
> my thinking is we probably want all of these to be warnings because it'd be 
> hard to explain why `std` is reserved but with a warning while `_Test` is 
> reserved but with an error.

Yeah, I'd treat them equally - while we could subset the reserved names and 
allow implementations to only use a subset (while leaving the rest as an error 
for both implementations and consumers alike) that doesn't feel in keeping with 
the purpose of these names - to be usable by /someone/ and so necessary to 
allow them to be used.

(hmm - there's some discussion in the description about the fact that this 
error was already suppressed in "system headers" - why was that suppression 
inadequate for system implementation modules? (& does that suppression for 
reserved names risk being over-broad, since every third party library installed 
on a system is generally considered a "system header", even if they aren't part 
of the implementation?))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146986

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

[PATCH] D146986: Downgrade reserved module identifier error into a warning

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: ChuanqiXu, dblaikie, Mordante, iains.
Herald added a project: All.
aaron.ballman requested review of this revision.
Herald added a project: clang.

Any project that wants to `import std;` potentially needs to be able to build a 
module that does `export std;`. We silenced the error diagnostic if the module 
identified itself as a system header, but this isn't quite good enough, what we 
really need is a way to identify a system module. It would be nice for that 
feature to be shared among the major implementations, so this downgrades the 
diagnostic from an error to a warning temporarily to give implementers time to 
determine what that mechanism will look like. We may convert this warning back 
into an error in a future release of Clang, but it's not guaranteed we will do 
so.

Fixes https://github.com/llvm/llvm-project/issues/61446


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146986

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaModule.cpp
  clang/test/Modules/reserved-names-1.cpp

Index: clang/test/Modules/reserved-names-1.cpp
===
--- clang/test/Modules/reserved-names-1.cpp
+++ clang/test/Modules/reserved-names-1.cpp
@@ -1,34 +1,35 @@
-// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %s
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %s
 
 // expected-note@1 15{{add 'module;' to the start of the file to introduce a global module fragment}}
 
-module std;// expected-error {{'std' is a reserved name for a module}}
-module _Test;  // expected-error {{'_Test' is a reserved name for a module}} \
+module std;// loud-warning {{'std' is a reserved name for a module}}
+module _Test;  // loud-warning {{'_Test' is a reserved name for a module}} \
   expected-error {{module declaration must occur at the start of the translation unit}}
 module module; // expected-error {{'module' is an invalid name for a module}} \
   expected-error {{module declaration must occur at the start of the translation unit}}
-module std0;   // expected-error {{'std0' is a reserved name for a module}} \
+module std0;   // loud-warning {{'std0' is a reserved name for a module}} \
   expected-error {{module declaration must occur at the start of the translation unit}}
 
 export module module; // expected-error {{'module' is an invalid name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
 export module import; // expected-error {{'import' is an invalid name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module _Test;  // expected-error {{'_Test' is a reserved name for a module}} \
+export module _Test;  // loud-warning {{'_Test' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module __test; // expected-error {{'__test' is a reserved name for a module}} \
+export module __test; // loud-warning {{'__test' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module te__st; // expected-error {{'te__st' is a reserved name for a module}} \
+export module te__st; // loud-warning {{'te__st' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module std;// expected-error {{'std' is a reserved name for a module}} \
+export module std;// loud-warning {{'std' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module std.foo;// expected-error {{'std' is a reserved name for a module}} \
+export module std.foo;// loud-warning {{'std' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module std0;   // expected-error {{'std0' is a reserved name for a module}} \
+export module std0;   // loud-warning {{'std0' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the translation unit}}
-export module std100; // expected-error {{'std100' is a reserved name for a module}} \
+export module std100; // loud-warning {{'std100' is a reserved name for a module}} \
  expected-error {{module declaration must occur at the start of the