Re: [clang] acaf6b9 - [NFC] Add [[maybe_unused]] to avoid warning in gcc9

[chuanqi.xcq via cfe-commits]

Hi David,
 This is the reproduce link from godbolt: https://godbolt.org/z/nEc7WYKnq. It 
is weird that it requries `-Wunused-but-set-parameter` instead of 
`-Wunused-parameter`. The bug exists in all versions of GCC9. And it gets fixed 
in GCC10 and later. Personally, I feel it looks better to suppress the warning 
for GCC9.
Thanks,
Chuanqi
--
From:David Blaikie 
Send Time:2022年8月23日(星期二) 07:22
To:Chuanqi Xu ; Chuanqi Xu 
Cc:cfe-commits 
Subject:Re: [clang] acaf6b9 - [NFC] Add [[maybe_unused]] to avoid warning in 
gcc9
Seems like a bug in the GCC9 warning - any chance we can disable it?
(is it fixed in later versions of GCC?)
I can't seem to reproduce this with godbolt at least with basic
examples - it'd be good to know how bad the thing is we're working
around so we know if we want to keep working around it, or suppress
the warning for certainly compiler versions entirely.
On Thu, Aug 18, 2022 at 11:43 PM Chuanqi Xu via cfe-commits
 wrote:
>
>
> Author: Chuanqi Xu
> Date: 2022-08-19T14:43:22+08:00
> New Revision: acaf6b9dc07de3c12c8a1a55fd8674bca547a917
>
> URL: 
> https://github.com/llvm/llvm-project/commit/acaf6b9dc07de3c12c8a1a55fd8674bca547a917
> DIFF: 
> https://github.com/llvm/llvm-project/commit/acaf6b9dc07de3c12c8a1a55fd8674bca547a917.diff
>
> LOG: [NFC] Add [[maybe_unused]] to avoid warning in gcc9
>
> GCC9 may issue warning for the 'unused' parameters in if constexpr.
> This commit try to fix it by adding the [[maybe_unused]] attribute.
>
> Added:
>
>
> Modified:
> clang/include/clang/AST/RecursiveASTVisitor.h
>
> Removed:
>
>
>
> 
> diff --git a/clang/include/clang/AST/RecursiveASTVisitor.h 
> b/clang/include/clang/AST/RecursiveASTVisitor.h
> index 91baad51b26e..bd7fadb87c5f 100644
> --- a/clang/include/clang/AST/RecursiveASTVisitor.h
> +++ b/clang/include/clang/AST/RecursiveASTVisitor.h
> @@ -73,7 +73,8 @@ struct has_same_member_pointer_type (U::*)(P...)>
> /// are pointers to the same non-static member function.
> template 
> LLVM_ATTRIBUTE_ALWAYS_INLINE LLVM_ATTRIBUTE_NODEBUG auto
> -isSameMethod(FirstMethodPtrTy FirstMethodPtr, SecondMethodPtrTy 
> SecondMethodPtr)
> +isSameMethod([[maybe_unused]] FirstMethodPtrTy FirstMethodPtr,
> + [[maybe_unused]] SecondMethodPtrTy SecondMethodPtr)
> -> bool {
> if constexpr (has_same_member_pointer_type SecondMethodPtrTy>::value)
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang] 6ed67cc - [Coroutines] Remove -fcoroutines-ts

[chuanqi.xcq via cfe-commits]

Hi Bruno,
 We talked about removing `-fcoroutines-ts` in 
https://github.com/llvm/llvm-project/issues/59110
and https://reviews.llvm.org/D108697. I raised the example you used here. And 
the conclusion
is that this is the clang's policy for `-f*-ts` options. The same thing happens 
for -fconcepts-ts and
-fmodules-ts. From my understanding, this is because we (clang) want less 
dialects and we want to
focus on the form `-std=*`.
 > If my understanding now is right, `-std=c++17 -fcoroutines` should work, 
 > right?
`clang -std=c++17 -fcoroutines` shouldn't work. Since it is the same with 
`-fcoroutines-ts`.
But if you want or you can't update to `-std=c++20` quickly, you can use
`clang -std=c++17 -Xclang -fcoroutines` for the period of transition. But the 
`Xclang` options are
for developers instead of the users. So we (the users) should update
to `-std=c++20` or higher to use coroutines finally.
Thanks,
Chuanqi
--
From:Bruno Cardoso Lopes 
Send Time:2023年2月28日(星期二) 06:51
To:Chuanqi Xu ; Chuanqi Xu 
Cc:cfe-commits 
Subject:Re: [clang] 6ed67cc - [Coroutines] Remove -fcoroutines-ts
> I understand that the name "coroutines-ts" isn't meaningful these
> days, but it also sounds like this commit does more than remove the
> flag, it caps useful functionality. How are users supposed to use
> c++17 with coroutines now? It's very common in our codebase and we
> have users relying on it.
Looks like I misread your patch, sorry! If my understanding now is
right, `-std=c++17 -fcoroutines` should work, right? We should
probably add an extra test in `clang/test/Driver/coroutines.cpp` that
just test that (we currently just test when it's not).
Cheers,
-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[chuanqi.xcq via cfe-commits]

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 
Send Time:2023年4月13日(星期四) 20:01
To:Chuanqi Xu ; Chuanqi Xu 
Cc:cfe-commits 
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
 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..0
> --- 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@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 oc

Re: [clang] 9791b58 - [C++20 Modules] Don't create global module fragment for extern linkage declaration in GMF already

[chuanqi.xcq via cfe-commits]

Hi Richard,

Thanks for reporting this! I found the reason that why it didn't get tested 
previously is that we didn't use `extern "C"/"C++"` actually since we 
implemented partitions internally. Sorry for confusing. I would try to work on 
it.

Thanks,
Chuanqi


--
From:Richard Smith 
Send Time:2021年12月11日(星期六) 08:28
To:Chuanqi Xu ; Chuanqi Xu 
Cc:cfe-commits 
Subject:Re: [clang] 9791b58 - [C++20 Modules] Don't create global module 
fragment for extern linkage declaration in GMF already

On Wed, 8 Dec 2021 at 21:56, Chuanqi Xu via cfe-commits 
 wrote:

 Author: Chuanqi Xu
 Date: 2021-12-09T13:55:15+08:00
 New Revision: 9791b589516b644a6273607b46a9c6661993d667

 URL: 
https://github.com/llvm/llvm-project/commit/9791b589516b644a6273607b46a9c6661993d667
 DIFF: 
https://github.com/llvm/llvm-project/commit/9791b589516b644a6273607b46a9c6661993d667.diff

 LOG: [C++20 Modules] Don't create global module fragment for extern linkage 
declaration in GMF already

 Previously we would create global module fragment for extern linkage
 declaration which is alreday in global module fragment. However, it is
 clearly redundant to do so. This patch would check if the extern linkage
 declaration are already in GMF before we create a GMF for it.

I'm still seeing some problems even after this patch -- linkage spec 
declarations within the purview of the module still create new global module 
fragments, and then the serialization code seems to sometimes get confused 
because we have multiple submodules with the same name "". Perhaps we 
should reuse an existing global module fragment if there is one, even when 
inside the purview of the module?

Here's an example end-to-end testcase for which I'm seeing an assertion 
failure; I've not got a reduced testcase yet:

$ cat say_hello.cppm
module;
#include 
#include 
export module Hello;
export void SayHello
  (std::string_view const &name)
{
  std::cout << "Hello " << name << "!\n";
}

$ cat hello.cpp
#include 
import Hello;
int main() {
  SayHello("world");
  return 0;
}

$ ./build/bin/clang++ say_hello.cppm --precompile -o say_hello.pcm -std=c++20
$ ./build/bin/clang++ say_hello.cppm hello.cpp -fmodule-file=say_hello.pcm 
-std=c++20

 Added: 
 clang/test/CXX/module/module.unit/p7/Inputs/h7.h
 clang/test/CXX/module/module.unit/p7/t7.cpp

 Modified: 
 clang/include/clang/Sema/Sema.h
 clang/lib/Sema/SemaDeclCXX.cpp

 Removed: 



 

 diff  --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
 index a12dd4db66c13..73c5ad1dd7acf 100644
 --- a/clang/include/clang/Sema/Sema.h
 +++ b/clang/include/clang/Sema/Sema.h
 @@ -,6 +,12 @@ class Sema final {
  return ModuleScopes.empty() ? nullptr : ModuleScopes.back().Module;
}

 +  /// Helper function to judge if we are in module purview.
 +  /// Return false if we are not in a module.
 +  bool isCurrentModulePurview() const {
 +return getCurrentModule() ? getCurrentModule()->isModulePurview() : false;
 +  }
 +
/// Enter the scope of the global module.
Module *PushGlobalModuleFragment(SourceLocation BeginLoc, bool IsImplicit);
/// Leave the scope of the global module.

 diff  --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
 index 4a0eda2a700fe..3f6bde1b9ed6a 100644
 --- a/clang/lib/Sema/SemaDeclCXX.cpp
 +++ b/clang/lib/Sema/SemaDeclCXX.cpp
 @@ -16153,7 +16153,10 @@ Decl *Sema::ActOnStartLinkageSpecification(Scope *S, 
SourceLocation ExternLoc,
///   - ...
///   - appears within a linkage-specification,
///   it is attached to the global module.
 -  if (getLangOpts().CPlusPlusModules && getCurrentModule()) {
 +  ///
 +  /// If the declaration is already in global module fragment, we don't
 +  /// need to attach it again.
 +  if (getLangOpts().CPlusPlusModules && isCurrentModulePurview()) {
  Module *GlobalModule =
  PushGlobalModuleFragment(ExternLoc, /*IsImplicit=*/true);
  D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
 @@ -16177,7 +16180,11 @@ Decl *Sema::ActOnFinishLinkageSpecification(Scope *S,
  LSDecl->setRBraceLoc(RBraceLoc);
}

 -  if (getLangOpts().CPlusPlusModules && getCurrentModule())
 +  // If the current module doesn't has Parent, it implies that the
 +  // LinkageSpec isn't in the module created by itself. So we don't
 +  // need to pop it.
 +  if (getLangOpts().CPlusPlusModules && getCurrentModule() &&
 +  getCurrentModule()->isGlobalModule() && getCurrentModule()->Parent)
  PopGlobalModuleFragment();

PopDeclContext();

 diff  --git a/clang/test/CXX/module/module.unit/p7/Inputs/h7.h 
b/clang/test/CXX/module/module.unit/p7/Inputs/h7.h
 new file mode 100644
 index 0..cd3b6706d561c
 --- /dev/null
 +++ b/clang/test/CXX/module/module.unit/p7/Inputs/h7.h
 @@ -0,0 +1,10 @@
 +#ifndef H7_H
 +#define H7_H
 +
 +extern "C+

Re: [clang] 9791b58 - [C++20 Modules] Don't create global module fragment for extern linkage declaration in GMF already

[chuanqi.xcq via cfe-commits]

Hi Richard,

   Sorry for disturbing. I sent https://reviews.llvm.org/D115610 to try to fix 
this. 

BTW, the example you posted might not work with libstdc++, here is a bug I 
reported: https://github.com/llvm/llvm-project/issues/51873

Thanks,
Chuanqi



--
From:chuanqi.xcq 
Send Time:2021年12月13日(星期一) 10:14
To:Chuanqi Xu ; Richard Smith 
Cc:cfe-commits 
Subject:Re: [clang] 9791b58 - [C++20 Modules] Don't create global module 
fragment for extern linkage declaration in GMF already

Hi Richard,

Thanks for reporting this! I found the reason that why it didn't get tested 
previously is that we didn't use `extern "C"/"C++"` actually since we 
implemented partitions internally. Sorry for confusing. I would try to work on 
it.

Thanks,
Chuanqi

--
From:Richard Smith 
Send Time:2021年12月11日(星期六) 08:28
To:Chuanqi Xu ; Chuanqi Xu 
Cc:cfe-commits 
Subject:Re: [clang] 9791b58 - [C++20 Modules] Don't create global module 
fragment for extern linkage declaration in GMF already

On Wed, 8 Dec 2021 at 21:56, Chuanqi Xu via cfe-commits 
 wrote:

 Author: Chuanqi Xu
 Date: 2021-12-09T13:55:15+08:00
 New Revision: 9791b589516b644a6273607b46a9c6661993d667

 URL: 
https://github.com/llvm/llvm-project/commit/9791b589516b644a6273607b46a9c6661993d667
 DIFF: 
https://github.com/llvm/llvm-project/commit/9791b589516b644a6273607b46a9c6661993d667.diff

 LOG: [C++20 Modules] Don't create global module fragment for extern linkage 
declaration in GMF already

 Previously we would create global module fragment for extern linkage
 declaration which is alreday in global module fragment. However, it is
 clearly redundant to do so. This patch would check if the extern linkage
 declaration are already in GMF before we create a GMF for it.

I'm still seeing some problems even after this patch -- linkage spec 
declarations within the purview of the module still create new global module 
fragments, and then the serialization code seems to sometimes get confused 
because we have multiple submodules with the same name "". Perhaps we 
should reuse an existing global module fragment if there is one, even when 
inside the purview of the module?

Here's an example end-to-end testcase for which I'm seeing an assertion 
failure; I've not got a reduced testcase yet:

$ cat say_hello.cppm
module;
#include 
#include 
export module Hello;
export void SayHello
  (std::string_view const &name)
{
  std::cout << "Hello " << name << "!\n";
}

$ cat hello.cpp
#include 
import Hello;
int main() {
  SayHello("world");
  return 0;
}

$ ./build/bin/clang++ say_hello.cppm --precompile -o say_hello.pcm -std=c++20
$ ./build/bin/clang++ say_hello.cppm hello.cpp -fmodule-file=say_hello.pcm 
-std=c++20

 Added: 
 clang/test/CXX/module/module.unit/p7/Inputs/h7.h
 clang/test/CXX/module/module.unit/p7/t7.cpp

 Modified: 
 clang/include/clang/Sema/Sema.h
 clang/lib/Sema/SemaDeclCXX.cpp

 Removed: 



 

 diff  --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
 index a12dd4db66c13..73c5ad1dd7acf 100644
 --- a/clang/include/clang/Sema/Sema.h
 +++ b/clang/include/clang/Sema/Sema.h
 @@ -,6 +,12 @@ class Sema final {
  return ModuleScopes.empty() ? nullptr : ModuleScopes.back().Module;
}

 +  /// Helper function to judge if we are in module purview.
 +  /// Return false if we are not in a module.
 +  bool isCurrentModulePurview() const {
 +return getCurrentModule() ? getCurrentModule()->isModulePurview() : false;
 +  }
 +
/// Enter the scope of the global module.
Module *PushGlobalModuleFragment(SourceLocation BeginLoc, bool IsImplicit);
/// Leave the scope of the global module.

 diff  --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
 index 4a0eda2a700fe..3f6bde1b9ed6a 100644
 --- a/clang/lib/Sema/SemaDeclCXX.cpp
 +++ b/clang/lib/Sema/SemaDeclCXX.cpp
 @@ -16153,7 +16153,10 @@ Decl *Sema::ActOnStartLinkageSpecification(Scope *S, 
SourceLocation ExternLoc,
///   - ...
///   - appears within a linkage-specification,
///   it is attached to the global module.
 -  if (getLangOpts().CPlusPlusModules && getCurrentModule()) {
 +  ///
 +  /// If the declaration is already in global module fragment, we don't
 +  /// need to attach it again.
 +  if (getLangOpts().CPlusPlusModules && isCurrentModulePurview()) {
  Module *GlobalModule =
  PushGlobalModuleFragment(ExternLoc, /*IsImplicit=*/true);
  D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
 @@ -16177,7 +16180,11 @@ Decl *Sema::ActOnFinishLinkageSpecification(Scope *S,
  LSDecl->setRBraceLoc(RBraceLoc);
}

 -  if (getLangOpts().CPlusPlusModules && getCurrentModule())
 +  // If the current module doesn't has Parent, it implies that the
 +  // LinkageSpec isn't in the module created