Re: [clang] c1f7636 - [C++20] [Modules] Continue parsing after we found reserved module names

Thu Apr 13 18:54:44 PDT 2023

Hi Aaron,
 I don't think we need to backport this to 16.x. Since the previous patch https://reviews.llvm.org/D146986 doesn't get backported too. The patch itself is mainly for the implementation of std modules in libcxx. And the std modules in libcxx should target 17.x as far as I know. So it looks not necessary to backport this.
Thanks,
Chuanqi
------------------------------------------------------------------
From:Aaron Ballman <aaron at aaronballman.com>
Send Time:2023年4月13日(星期四) 20:01
To:Chuanqi Xu <yedeng.yd at linux.alibaba.com>; Chuanqi Xu <llvmlistbot at llvm.org>
Cc:cfe-commits <cfe-commits at lists.llvm.org>
Subject:Re: [clang] c1f7636 - [C++20] [Modules] Continue parsing after we found reserved module names
On Thu, Apr 13, 2023 at 3:14 AM Chuanqi Xu via cfe-commits
<cfe-commits at lists.llvm.org> wrote:
>
>
> Author: Chuanqi Xu
> Date: 2023-04-13T15:14:34+08:00
> New Revision: c1f76363e0db41ab6eb9ebedd687ee098491e9b7
>
> URL: https://github.com/llvm/llvm-project/commit/c1f76363e0db41ab6eb9ebedd687ee098491e9b7
> DIFF: https://github.com/llvm/llvm-project/commit/c1f76363e0db41ab6eb9ebedd687ee098491e9b7.diff
>
> LOG: [C++20] [Modules] Continue parsing after we found reserved module names
>
> Close https://github.com/llvm/llvm-project/issues/62112
>
> In the previous change, we'll stop parsing directly after we found
> reserved module names. But this may be too aggressive. This patch
> changes this. Note that the parsing will still be stopped if the module
> name is `module` or `import`.
Thank you for fixing this up! I think this should be backported to 16.0.2, WDYT?
~Aaron
>
> Added:
> clang/test/Modules/reserved-names-1.cppm
> clang/test/Modules/reserved-names-2.cppm
> clang/test/Modules/reserved-names-3.cppm
> clang/test/Modules/reserved-names-4.cppm
>
> Modified:
> clang/lib/Sema/SemaModule.cpp
>
> Removed:
> clang/test/Modules/reserved-names-1.cpp
> clang/test/Modules/reserved-names-2.cpp
> clang/test/Modules/reserved-names-3.cpp
> clang/test/Modules/reserved-names-4.cpp
>
>
> ################################################################################
> diff --git a/clang/lib/Sema/SemaModule.cpp b/clang/lib/Sema/SemaModule.cpp
> index 6c39cc0b44ca4..84a1fd854d804 100644
> --- a/clang/lib/Sema/SemaModule.cpp
> +++ b/clang/lib/Sema/SemaModule.cpp
> @@ -162,7 +162,8 @@ static bool DiagReservedModuleName(Sema &S, const IdentifierInfo *II,
> case Invalid:
> return S.Diag(Loc, diag::err_invalid_module_name) << II;
> case Reserved:
> - return S.Diag(Loc, diag::warn_reserved_module_name) << II;
> + S.Diag(Loc, diag::warn_reserved_module_name) << II;
> + return false;
> }
> llvm_unreachable("fell off a fully covered switch");
> }
> @@ -267,10 +268,8 @@ Sema::ActOnModuleDecl(SourceLocation StartLoc, SourceLocation ModuleLoc,
> if (!getSourceManager().isInSystemHeader(Path[0].second) &&
> (FirstComponentName == "std" ||
> (FirstComponentName.startswith("std") &&
> - llvm::all_of(FirstComponentName.drop_front(3), &llvm::isDigit)))) {
> + llvm::all_of(FirstComponentName.drop_front(3), &llvm::isDigit))))
> Diag(Path[0].second, diag::warn_reserved_module_name) << Path[0].first;
> - return nullptr;
> - }
>
> // Then test all of the components in the path to see if any of them are
> // using another kind of reserved or invalid identifier.
>
> diff --git a/clang/test/Modules/reserved-names-1.cpp b/clang/test/Modules/reserved-names-1.cpp
> deleted file mode 100644
> index a92c2244f1cb6..0000000000000
> --- a/clang/test/Modules/reserved-names-1.cpp
> +++ /dev/null
> @@ -1,46 +0,0 @@
> -// 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 at 1 15{{add 'module;' to the start of the file to introduce a global module fragment}}
> -
> -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; // 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; // 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; // 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; // 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; // 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;// 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; // 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 std1000000; // loud-warning {{'std1000000' is a reserved name for a module}} \
> - expected-error {{module declaration must occur at the start of the translation unit}}
> -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
> -// use of an invalid module-name identifier.
> -# 34 "reserved-names-1.cpp" 1 3
> -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 _Test.import; // expected-error {{'import' is an invalid name for a module}} \
> - expected-error {{module declaration must occur at the start of the translation unit}}
> -# 39 "reserved-names-1.cpp" 2 3
> -
> -// We can still use a reserved name on imoport.
> -import std; // expected-error {{module 'std' not found}}
>
> diff --git a/clang/test/Modules/reserved-names-1.cppm b/clang/test/Modules/reserved-names-1.cppm
> new file mode 100644
> index 0000000000000..e780f1e35b3b7
> --- /dev/null
> +++ b/clang/test/Modules/reserved-names-1.cppm
> @@ -0,0 +1,154 @@
> +// RUN: rm -rf %t
> +// RUN: mkdir -p %t
> +// RUN: split-file %s %t
> +
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/module.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %t/module.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/import.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %t/import.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/_Test.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -DNODIAGNOSTICS -Wno-reserved-module-identifier %t/_Test.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/__test.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -DNODIAGNOSTICS -Wno-reserved-module-identifier %t/__test.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/te__st.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -DNODIAGNOSTICS -Wno-reserved-module-identifier %t/te__st.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/std.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %t/std.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/std.foo.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %t/std.foo.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/std0.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %t/std0.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/std1000000.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %t/std1000000.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/module.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %t/module.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/import.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %t/import.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/_Test.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -DNODIAGNOSTICS -Wno-reserved-module-identifier %t/_Test.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/__test.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -DNODIAGNOSTICS -Wno-reserved-module-identifier %t/__test.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/te__st.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -DNODIAGNOSTICS -Wno-reserved-module-identifier %t/te__st.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/std.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -DNODIAGNOSTICS -Wno-reserved-module-identifier %t/std.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/std.foo.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -DNODIAGNOSTICS -Wno-reserved-module-identifier %t/std.foo.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/std0.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -DNODIAGNOSTICS -Wno-reserved-module-identifier %t/std0.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/std1000000.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -DNODIAGNOSTICS -Wno-reserved-module-identifier %t/std1000000.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/should_diag._Test.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -DNODIAGNOSTICS -Wno-reserved-module-identifier %t/should_diag._Test.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/system-module.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %t/system-module.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/system._Test.import.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %t/system._Test.import.cppm
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,loud %t/user.cpp
> +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected -Wno-reserved-module-identifier %t/user.cpp
> +
> +//--- module.cpp
> +module module; // expected-error {{'module' is an invalid name for a module}}
> +
> +//--- import.cpp
> +module import; // expected-error {{'import' is an invalid name for a module}}
> +
> +//--- _Test.cpp
> +module _Test; // loud-warning {{'_Test' is a reserved name for a module}} \
> + // expected-error {{module '_Test' not found}}
> +
> +//--- __test.cpp
> +module __test; // loud-warning {{'__test' is a reserved name for a module}} \
> + // expected-error {{module '__test' not found}}
> +
> +//--- te__st.cpp
> +module te__st; // loud-warning {{'te__st' is a reserved name for a module}} \
> + // expected-error {{module 'te__st' not found}}
> +
> +//--- std.cpp
> +module std; // loud-warning {{'std' is a reserved name for a module}} \
> + // expected-error {{module 'std' not found}}
> +
> +//--- std.foo.cpp
> +module std.foo; // loud-warning {{'std' is a reserved name for a module}} \
> + // expected-error {{module 'std.foo' not found}}
> +
> +//--- std0.cpp
> +module std0; // loud-warning {{'std0' is a reserved name for a module}} \
> + // expected-error {{module 'std0' not found}}
> +
> +//--- std1000000.cpp
> +module std1000000; // loud-warning {{'std1000000' is a reserved name for a module}} \
> + // expected-error {{module 'std1000000' not found}}
> +
> +//--- module.cppm
> +export module module; // expected-error {{'module' is an invalid name for a module}}
> +
> +//--- import.cppm
> +export module import; // expected-error {{'import' is an invalid name for a module}}
> +
> +//--- _Test.cppm
> +#ifdef NODIAGNOSTICS
> +// expected-no-diagnostics
> +#endif
> +export module _Test; // loud-warning {{'_Test' is a reserved name for a module}}
> +
> +//--- __test.cppm
> +#ifdef NODIAGNOSTICS
> +// expected-no-diagnostics
> +#endif
> +export module __test; // loud-warning {{'__test' is a reserved name for a module}}
> +export int a = 43;
> +
> +//--- te__st.cppm
> +#ifdef NODIAGNOSTICS
> +// expected-no-diagnostics
> +#endif
> +export module te__st; // loud-warning {{'te__st' is a reserved name for a module}}
> +export int a = 43;
> +
> +//--- std.cppm
> +#ifdef NODIAGNOSTICS
> +// expected-no-diagnostics
> +#endif
> +export module std; // loud-warning {{'std' is a reserved name for a module}}
> +
> +export int a = 43;
> +
> +//--- std.foo.cppm
> +#ifdef NODIAGNOSTICS
> +// expected-no-diagnostics
> +#endif
> +export module std.foo;// loud-warning {{'std' is a reserved name for a module}}
> +
> +//--- std0.cppm
> +#ifdef NODIAGNOSTICS
> +// expected-no-diagnostics
> +#endif
> +export module std0; // loud-warning {{'std0' is a reserved name for a module}}
> +
> +//--- std1000000.cppm
> +#ifdef NODIAGNOSTICS
> +// expected-no-diagnostics
> +#endif
> +export module std1000000; // loud-warning {{'std1000000' is a reserved name for a module}}
> +
> +//--- should_diag._Test.cppm
> +#ifdef NODIAGNOSTICS
> +// expected-no-diagnostics
> +#endif
> +export module should_diag._Test; // loud-warning {{'_Test' is a reserved name for a module}}
> +
> +//--- system-module.cppm
> +// Show that being in a system header doesn't save you from diagnostics about
> +// use of an invalid module-name identifier.
> +# 34 "reserved-names-1.cpp" 1 3
> +export module module; // expected-error {{'module' is an invalid name for a module}}
> +
> +//--- system._Test.import.cppm
> +# 34 "reserved-names-1.cpp" 1 3
> +export module _Test.import; // expected-error {{'import' is an invalid name for a module}}
> +
> +//--- user.cpp
> +// We can still use a reserved name on imoport.
> +import std; // expected-error {{module 'std' not found}}
>
> diff --git a/clang/test/Modules/reserved-names-2.cpp b/clang/test/Modules/reserved-names-2.cppm
> similarity index 100%
> rename from clang/test/Modules/reserved-names-2.cpp
> rename to clang/test/Modules/reserved-names-2.cppm
>
> diff --git a/clang/test/Modules/reserved-names-3.cpp b/clang/test/Modules/reserved-names-3.cppm
> similarity index 100%
> rename from clang/test/Modules/reserved-names-3.cpp
> rename to clang/test/Modules/reserved-names-3.cppm
>
> diff --git a/clang/test/Modules/reserved-names-4.cpp b/clang/test/Modules/reserved-names-4.cppm
> similarity index 100%
> rename from clang/test/Modules/reserved-names-4.cpp
> rename to clang/test/Modules/reserved-names-4.cppm
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230414/6137afe1/attachment-0001.html>