[PATCH] D131388: [docs] Add "Standard C++ Modules"

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 456244.
ChuanqiXu marked 5 inline comments as done.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D131388

Files:
  clang/docs/StandardCPlusPlusModules.rst
  clang/docs/index.rst

Index: clang/docs/index.rst
===
--- clang/docs/index.rst
+++ clang/docs/index.rst
@@ -40,6 +40,7 @@
SafeStack
ShadowCallStack
SourceBasedCodeCoverage
+   StandardCPlusPlusModules
Modules
MSVCCompatibility
MisExpect
Index: clang/docs/StandardCPlusPlusModules.rst
===
--- /dev/null
+++ clang/docs/StandardCPlusPlusModules.rst
@@ -0,0 +1,835 @@
+
+Standard C++ Modules
+
+
+.. contents::
+   :local:
+
+Introduction
+
+
+The term ``modules`` has a lot of meanings. For the users of Clang, modules may
+refer to ``Objective-C Modules``, ``Clang C++ Modules`` (or ``Clang Header Modules``,
+etc.) or ``Standard C++ Modules``. The implementation of all these kinds of modules in Clang
+has a lot of shared code, but from the perspective of users, their semantics and
+command line interfaces are very different. This document focuses on
+an introduction of how to use standard C++ modules in Clang.
+
+There is already a detailed document about `Clang modules `_, it
+should be helpful to read `Clang modules `_ if you want to know
+more about the general idea of modules. Since standard C++ modules have different semantics
+(and work flows) from `Clang modules`, this page describes the background and use of
+Clang with standard C++ modules.
+
+Modules exist in two forms in the C++ Language Specification. They can refer to
+either "Named Modules" or to "Header Units". This document covers both forms.
+
+Standard C++ Named modules
+==
+
+This document was intended to be a manual first and foremost, however, we consider it helpful to
+introduce some language background here for readers who are not familiar with
+the new language feature. This document is not intended to be a language
+tutorial; it will only introduce necessary concepts about the
+structure and building of the project.
+
+Background and terminology
+--
+
+Modules
+~~~
+
+In this document, the term ``Modules``/``modules`` refers to standard C++ modules
+feature if it is not decorated by ``Clang``.
+
+Clang Modules
+~
+
+In this document, the term ``Clang Modules``/``Clang modules`` refer to Clang
+c++ modules extension. These are also known as ``Clang header modules``,
+``Clang module map modules`` or ``Clang c++ modules``.
+
+Module and module unit
+~~
+
+A module consists of one or more module units. A module unit is a special
+translation unit. Every module unit must have a module declaration. The syntax
+of the module declaration is:
+
+.. code-block:: c++
+
+  [export] module module_name[:partition_name];
+
+Terms enclosed in ``[]`` are optional. The syntax of ``module_name`` and ``partition_name``
+in regex form corresponds to ``[a-zA-Z_][a-zA-Z_0-9\.]*``. In particular, a literal dot ``.``
+in the name has no semantic meaning (e.g. implying a hierarchy).
+
+In this document, module units are classified into:
+
+* Primary module interface unit.
+
+* Module implementation unit.
+
+* Module interface partition unit.
+
+* Internal module partition unit.
+
+A primary module interface unit is a module unit whose module declaration is
+``export module module_name;``. The ``module_name`` here denotes the name of the
+module. A module should have one and only one primary module interface unit.
+
+A module implementation unit is a module unit whose module declaration is
+``module module_name;``. A module could have multiple module implementation
+units with the same declaration.
+
+A module interface partition unit is a module unit whose module declaration is
+``export module module_name:partition_name;``. The ``partition_name`` should be
+unique within any given module.
+
+An internal module partition unit is a module unit whose module declaration
+is ``module module_name:partition_name;``. The ``partition_name`` should be
+unique within any given module.
+
+In this document, we use the following umbrella terms:
+
+* A ``module interface unit`` refers to either a ``primary module interface unit``
+  or a ``module interface partition unit``.
+
+* An ``importable module unit`` refers to either a ``module interface unit``
+  or a ``internal module partition unit``.
+
+* A ``module partition unit`` refers to either a ``module interface partition unit``
+  or a ``internal module partition unit``.
+
+Built Module Interface file
+~~~
+
+A ``Built Module Interface file`` stands for the precompiled result of an importable module unit.
+It is also called the acron

[PATCH] D131388: [docs] Add "Standard C++ Modules"

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 456254.
ChuanqiXu added a comment.

Add contents for "When precompiling a module in one directory and then moving a 
header included in the global module fragment from the source file, things will 
break."


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

https://reviews.llvm.org/D131388

Files:
  clang/docs/StandardCPlusPlusModules.rst
  clang/docs/index.rst

Index: clang/docs/index.rst
===
--- clang/docs/index.rst
+++ clang/docs/index.rst
@@ -40,6 +40,7 @@
SafeStack
ShadowCallStack
SourceBasedCodeCoverage
+   StandardCPlusPlusModules
Modules
MSVCCompatibility
MisExpect
Index: clang/docs/StandardCPlusPlusModules.rst
===
--- /dev/null
+++ clang/docs/StandardCPlusPlusModules.rst
@@ -0,0 +1,835 @@
+
+Standard C++ Modules
+
+
+.. contents::
+   :local:
+
+Introduction
+
+
+The term ``modules`` has a lot of meanings. For the users of Clang, modules may
+refer to ``Objective-C Modules``, ``Clang C++ Modules`` (or ``Clang Header Modules``,
+etc.) or ``Standard C++ Modules``. The implementation of all these kinds of modules in Clang
+has a lot of shared code, but from the perspective of users, their semantics and
+command line interfaces are very different. This document focuses on
+an introduction of how to use standard C++ modules in Clang.
+
+There is already a detailed document about `Clang modules `_, it
+should be helpful to read `Clang modules `_ if you want to know
+more about the general idea of modules. Since standard C++ modules have different semantics
+(and work flows) from `Clang modules`, this page describes the background and use of
+Clang with standard C++ modules.
+
+Modules exist in two forms in the C++ Language Specification. They can refer to
+either "Named Modules" or to "Header Units". This document covers both forms.
+
+Standard C++ Named modules
+==
+
+This document was intended to be a manual first and foremost, however, we consider it helpful to
+introduce some language background here for readers who are not familiar with
+the new language feature. This document is not intended to be a language
+tutorial; it will only introduce necessary concepts about the
+structure and building of the project.
+
+Background and terminology
+--
+
+Modules
+~~~
+
+In this document, the term ``Modules``/``modules`` refers to standard C++ modules
+feature if it is not decorated by ``Clang``.
+
+Clang Modules
+~
+
+In this document, the term ``Clang Modules``/``Clang modules`` refer to Clang
+c++ modules extension. These are also known as ``Clang header modules``,
+``Clang module map modules`` or ``Clang c++ modules``.
+
+Module and module unit
+~~
+
+A module consists of one or more module units. A module unit is a special
+translation unit. Every module unit must have a module declaration. The syntax
+of the module declaration is:
+
+.. code-block:: c++
+
+  [export] module module_name[:partition_name];
+
+Terms enclosed in ``[]`` are optional. The syntax of ``module_name`` and ``partition_name``
+in regex form corresponds to ``[a-zA-Z_][a-zA-Z_0-9\.]*``. In particular, a literal dot ``.``
+in the name has no semantic meaning (e.g. implying a hierarchy).
+
+In this document, module units are classified into:
+
+* Primary module interface unit.
+
+* Module implementation unit.
+
+* Module interface partition unit.
+
+* Internal module partition unit.
+
+A primary module interface unit is a module unit whose module declaration is
+``export module module_name;``. The ``module_name`` here denotes the name of the
+module. A module should have one and only one primary module interface unit.
+
+A module implementation unit is a module unit whose module declaration is
+``module module_name;``. A module could have multiple module implementation
+units with the same declaration.
+
+A module interface partition unit is a module unit whose module declaration is
+``export module module_name:partition_name;``. The ``partition_name`` should be
+unique within any given module.
+
+An internal module partition unit is a module unit whose module declaration
+is ``module module_name:partition_name;``. The ``partition_name`` should be
+unique within any given module.
+
+In this document, we use the following umbrella terms:
+
+* A ``module interface unit`` refers to either a ``primary module interface unit``
+  or a ``module interface partition unit``.
+
+* An ``importable module unit`` refers to either a ``module interface unit``
+  or a ``internal module partition unit``.
+
+* A ``module partition unit`` refers to either a ``module interface partition unit``
+  or a ``internal module partition unit``.
+
+Built Module Interface file
+~~~
+
+A ``Built Module I

[PATCH] D131388: [docs] Add "Standard C++ Modules"

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D131388#3753522 , @aaronmondal 
wrote:

> @ChuanqiXu Thanks a lot for this!
>
> We are currently implementing this module logic in the Bazel ruleset rules_ll 
> .
> This page helps clarify the differences between clang modules and C++ modules 
> a lot. If I knew about this earlier we'd probably have saved a lot of time 😅

Yeah, the reason why I want to land this faster is that I've seen some people 
confused about this too.

(I've watched rules_ll for releases. So it will be better to *release* it if 
you guys get something done)

> After reading this doc, a few things came to my mind. I can add a patch for 
> this as a follow-up diff when I better understand how things work.
>
> - The path logic that references the source files in BMIs is very important. 
> When precompiling a module in one directory and then moving a header included 
> in the global module fragment from the source file, things will break.

Yes and I mentioned it implicitly that we can workaround it by `-Xclang 
-fmodules-embed-all-files` options. And I add a paragraph at the end of `Source 
content consistency` section to make it explicitly. Would you like to take a 
look?

> - Clang modulemaps are cool, but for build systems that have to declare every 
> output, implicit modulemaps seem like a bad idea. Clang's caching system, 
> even with custom cache paths seems like a bad idea as well. It is probably 
> helpful to mention caveats of implicit modules and how to disable them (or 
> link to the Clang Module docs for this).
> - The doc describes how to build e.g. an `iostream` module using the C++ 
> semantics, but doesn't mention that AFAIK there is no "native" C++ Module 
> build for libcxx yet. One would currently actually use Clang modules with 
> implicit modulemaps. It may be helpful to mention this to avoid confusion.

In fact, initially I want this document to be separate from clang module map 
modules. Although they share a lot in the implementation, they are totally 
different to the users. (Although they share some options too). To be clear, 
I'm not saying we'll abandon/disallow the mixed use of clang module map modules 
with standard c++ modules. The status quo is that every thing is still 
**unclear**. I'm not sure if they will work together correctly or what the cost 
we'll pay for to make them work together. So when I wrote this document, I 
intentionally skipped the part of how to use clang module map modules.

And I think it will be great to add a section "interaction with clang module 
map modules" once we have the use experience of mixed form.

> - Issue https://github.com/llvm/llvm-project/issues/56770 and 
> seems-to-be-a-consequence-issue 
> https://github.com/llvm/llvm-project/issues/57293 may be relevant apart from 
> the already mentioned issue about clang-scan-deps.

I'm not sure. And for this document, I feel it is fine to not include all the 
issues. Otherwise it may be a little bit scary to people who reads it first.

---

@aaronmondal @bbrown105 Are you happy to land this document in the current 
status? So that I can backport this to 15.x in time. (clang 15.x is going to be 
released in the next week). Then the readers could read the document for 15.x 
at 
https://releases.llvm.org/15.0.0/tools/clang/docs/StandardCPlusPlusModules.html.
 Then we can keep maintaining it further. This should be helpful for version 
controlling.




Comment at: clang/docs/StandardCPlusPlusModules.rst:243-244
+since ``--precompile`` option now would only run preprocessor, which is equal 
to `-E` now.
+If we still want the filename of an ``importable module unit`` ends with 
``.cpp`` instead of ``.cppm``,
+we could put ``-x c++-module`` in front of the file. For example,
+

bbrown105 wrote:
> Is there a tracking issue to revise this choice? I seem to recall that we 
> settled on recommending a different file extension, `*.ixx`, for at least 
> primary module interface units.
> 
> We didn't really discuss extensions for other units, but maybe we should if 
> different toolchains are going to have different expectations. Of course, 
> build systems can work around this sort of thing. But if the expectation of 
> clang is that, practically speaking, you'll probably be using a build system, 
> maybe the docs should clarify as much. We could document that parts of this 
> document are intended for people configuring or implementing build systems.
> 
> FYI @Bigcheese 
> Is there a tracking issue to revise this choice? I seem to recall that we 
> settled on recommending a different file extension, *.ixx, for at least 
> primary module interface units.

I remember there is not an existing one. And I just filed 
https://github.com/llvm/llvm-project/issues/57416. I think you can post the 
consensus of SG15 in that issue and we can follow it.

I edit the wording slightly (must -> should) and add a know

[PATCH] D132352: Introduce noread_thread_id to address the thread identification problem in coroutines

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked an inline comment as done.
ChuanqiXu added a comment.

In D132352#3755355 , @nikic wrote:

> Okay, this is a bit tricky because we have three different things:
>
> 1. The noread_thread_id attribute, the lack of which was causing issues with 
> intrinsics in the previous version
> 2. The meaning of the readnone (etc) attributes, which for pragmatic reasons 
> has to exclude thread IDs for now
> 3. The meaning of doesNotReadMemory() etc queries, which in the previous 
> version included thread ID accesses, but in the new version require a 
> separate call
>
> I think my question here would be why this did not stick with the previous 
> implementation approach that also affects doesNotReadMemory and AA queries 
> (and thus makes everything "automatically correct"), and only added the 
> noread_thread_id attribute to make intrinsic handling more precise?
>
> My general vision for this area was that after D130896 
> , we would add ThreadID as an additional 
> ModRef location, which gets removed for non-presplit-coroutines due to being 
> constant. This would follow the interpretation that the thread ID is part of 
> "memory" though, which kind of goes against the approach here.

I think the key point here is whether or not "thread_id" is part of "memory". 
According to 
https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015/48,
 we agree to treat "thread_id" is not part of memory. I feel the idea is to 
make these attributes more composable. (@nhaehnle ) And it looks like @jyknight 
@fhahn @rjmccall @efriedma tend to agree the direction if I don't misread. And 
your proposed solution should be available too. I think we need to get in 
consensus that whether or not "thread_id" is part of the "memory".

And another benefit of this method is that it is helpful to solve the potential 
similar problem in green threads (which is called stackful coroutines, or 
fibers). We mention about it here: 
https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015/28

(Some backgrounds for stackful coroutines: The stackful coroutines are not 
standard features and a vendor extension. the coroutine intrinsics in LLVM 
currently works for stackless coroutines. And the general implementation of 
stackful coroutine is not compiler dependent. The stackful coroutines save each 
register  manually when switching. So it is hard to detect stackful coroutines 
in compiler.)


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

https://reviews.llvm.org/D132352

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

[PATCH] D129833: Use @llvm.threadlocal.address intrinsic to access TLS

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D129833#3755135 , @nikic wrote:

> In D129833#3727881 , @ChuanqiXu 
> wrote:
>
>> And I am working on adding Align properties. But I meet problems since the 
>> alignment of threadlocal_address intrinsic depends on its argument so we 
>> can't set the alignment for its declaration and we probably need to set the 
>> alignment for its call, which means we need to set the alignment when in 
>> IRBuilder. Do you think this is good?
>
> I think that would be fine. Alternatively, it could be inferred in 
> InstCombine.

Thanks. I've filed an issue for it: 
https://github.com/llvm/llvm-project/issues/57438. (I'll fix it later if I had 
time)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129833

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

[PATCH] D132352: Introduce noread_thread_id to address the thread identification problem in coroutines

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D132352#3757415 , @rjmccall wrote:

> Stackful coroutine bodies should be straightforward to support on top of the 
> other work you've been doing, if anyone's actually interested in pursuing 
> them.  As far as the optimizer needs to know, a stackful coroutine function 
> is just like a presplit stackless coroutine except that calls and returns 
> work normally and it's never split.  Because it's never split, the backends 
> would need to understand that they can't arbitrarily reorder TLS 
> materializations and so on in those functions, which would probably be the 
> most complicated piece of work there.  Otherwise, I think we'd just need to 
> mark stackful coroutine bodies with some new attribute and then change 
> `cannotReadDifferentThreadIDIfMoved` to check for that, the same way it 
> checks for presplit stackless coroutines.

As far as I understand, we can't mark stackful coroutine bodies with special 
attributes. It is slightly different from stackless coroutine. A stackless 
coroutine is a suspendable function. So we can mark the function. But the 
stackful coroutine is a thread in the user space actually. (Or we can think 
stackful coroutine as a stack instead of a function)  In another word, if a 
function A in a stackful coroutine calls another function B, then B lives in 
the stackful coroutine too. The stackful coroutine switches by user(library) 
implemented methods and they are not standardized so we can't even do hacks for 
them.

I don't pursue stackful coroutine too. I raise the example to show the idea of 
`noread_thread_id` may have some slight advantage.


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

https://reviews.llvm.org/D132352

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

[PATCH] D131388: [docs] Add "Standard C++ Modules"

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 456844.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D131388

Files:
  clang/docs/StandardCPlusPlusModules.rst
  clang/docs/index.rst

Index: clang/docs/index.rst
===
--- clang/docs/index.rst
+++ clang/docs/index.rst
@@ -40,6 +40,7 @@
SafeStack
ShadowCallStack
SourceBasedCodeCoverage
+   StandardCPlusPlusModules
Modules
MSVCCompatibility
MisExpect
Index: clang/docs/StandardCPlusPlusModules.rst
===
--- /dev/null
+++ clang/docs/StandardCPlusPlusModules.rst
@@ -0,0 +1,876 @@
+
+Standard C++ Modules
+
+
+.. contents::
+   :local:
+
+Introduction
+
+
+The term ``modules`` has a lot of meanings. For the users of Clang, modules may
+refer to ``Objective-C Modules``, ``Clang C++ Modules`` (or ``Clang Header Modules``,
+etc.) or ``Standard C++ Modules``. The implementation of all these kinds of modules in Clang
+has a lot of shared code, but from the perspective of users, their semantics and
+command line interfaces are very different. This document focuses on
+an introduction of how to use standard C++ modules in Clang.
+
+There is already a detailed document about `Clang modules `_, it
+should be helpful to read `Clang modules `_ if you want to know
+more about the general idea of modules. Since standard C++ modules have different semantics
+(and work flows) from `Clang modules`, this page describes the background and use of
+Clang with standard C++ modules.
+
+Modules exist in two forms in the C++ Language Specification. They can refer to
+either "Named Modules" or to "Header Units". This document covers both forms.
+
+Standard C++ Named modules
+==
+
+This document was intended to be a manual first and foremost, however, we consider it helpful to
+introduce some language background here for readers who are not familiar with
+the new language feature. This document is not intended to be a language
+tutorial; it will only introduce necessary concepts about the
+structure and building of the project.
+
+Background and terminology
+--
+
+Modules
+~~~
+
+In this document, the term ``Modules``/``modules`` refers to standard C++ modules
+feature if it is not decorated by ``Clang``.
+
+Clang Modules
+~
+
+In this document, the term ``Clang Modules``/``Clang modules`` refer to Clang
+c++ modules extension. These are also known as ``Clang header modules``,
+``Clang module map modules`` or ``Clang c++ modules``.
+
+Module and module unit
+~~
+
+A module consists of one or more module units. A module unit is a special
+translation unit. Every module unit must have a module declaration. The syntax
+of the module declaration is:
+
+.. code-block:: c++
+
+  [export] module module_name[:partition_name];
+
+Terms enclosed in ``[]`` are optional. The syntax of ``module_name`` and ``partition_name``
+in regex form corresponds to ``[a-zA-Z_][a-zA-Z_0-9\.]*``. In particular, a literal dot ``.``
+in the name has no semantic meaning (e.g. implying a hierarchy).
+
+In this document, module units are classified into:
+
+* Primary module interface unit.
+
+* Module implementation unit.
+
+* Module interface partition unit.
+
+* Internal module partition unit.
+
+A primary module interface unit is a module unit whose module declaration is
+``export module module_name;``. The ``module_name`` here denotes the name of the
+module. A module should have one and only one primary module interface unit.
+
+A module implementation unit is a module unit whose module declaration is
+``module module_name;``. A module could have multiple module implementation
+units with the same declaration.
+
+A module interface partition unit is a module unit whose module declaration is
+``export module module_name:partition_name;``. The ``partition_name`` should be
+unique within any given module.
+
+An internal module partition unit is a module unit whose module declaration
+is ``module module_name:partition_name;``. The ``partition_name`` should be
+unique within any given module.
+
+In this document, we use the following umbrella terms:
+
+* A ``module interface unit`` refers to either a ``primary module interface unit``
+  or a ``module interface partition unit``.
+
+* An ``importable module unit`` refers to either a ``module interface unit``
+  or a ``internal module partition unit``.
+
+* A ``module partition unit`` refers to either a ``module interface partition unit``
+  or a ``internal module partition unit``.
+
+Built Module Interface file
+~~~
+
+A ``Built Module Interface file`` stands for the precompiled result of an importable module unit.
+It is also called the acronym ``BMI`` genrally.
+
+Global module fragme

[PATCH] D131388: [docs] Add "Standard C++ Modules"

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D131388#3758898 , @aaronmondal 
wrote:

> I just noticed that there probably needs to be one change. The doc sometimes 
> describes `-fprebuilt-module-interface`, which is not a valid option. I think 
> the occurrences were meant to be `-fpbrebuilt-module-path`.

Oh, thanks for noticing. It is weird that I made such a mistake...


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

https://reviews.llvm.org/D131388

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

[PATCH] D131388: [docs] Add "Standard C++ Modules"

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

@bbrown105 I am going to land this today. it looks like you're not objecting it 
and we don't have the time to wait for your formal approval...


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

https://reviews.llvm.org/D131388

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

[PATCH] D131388: [docs] Add "Standard C++ Modules"

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.

I'm going to land this. Thanks for everyone who reviewed this!


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

https://reviews.llvm.org/D131388

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

[PATCH] D131388: [docs] Add "Standard C++ Modules"

[Chuanqi Xu via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb1d5af81249d: [docs] Add "Standard C++ Modules" 
(authored by ChuanqiXu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131388

Files:
  clang/docs/StandardCPlusPlusModules.rst
  clang/docs/index.rst

Index: clang/docs/index.rst
===
--- clang/docs/index.rst
+++ clang/docs/index.rst
@@ -40,6 +40,7 @@
SafeStack
ShadowCallStack
SourceBasedCodeCoverage
+   StandardCPlusPlusModules
Modules
MSVCCompatibility
MisExpect
Index: clang/docs/StandardCPlusPlusModules.rst
===
--- /dev/null
+++ clang/docs/StandardCPlusPlusModules.rst
@@ -0,0 +1,876 @@
+
+Standard C++ Modules
+
+
+.. contents::
+   :local:
+
+Introduction
+
+
+The term ``modules`` has a lot of meanings. For the users of Clang, modules may
+refer to ``Objective-C Modules``, ``Clang C++ Modules`` (or ``Clang Header Modules``,
+etc.) or ``Standard C++ Modules``. The implementation of all these kinds of modules in Clang
+has a lot of shared code, but from the perspective of users, their semantics and
+command line interfaces are very different. This document focuses on
+an introduction of how to use standard C++ modules in Clang.
+
+There is already a detailed document about `Clang modules `_, it
+should be helpful to read `Clang modules `_ if you want to know
+more about the general idea of modules. Since standard C++ modules have different semantics
+(and work flows) from `Clang modules`, this page describes the background and use of
+Clang with standard C++ modules.
+
+Modules exist in two forms in the C++ Language Specification. They can refer to
+either "Named Modules" or to "Header Units". This document covers both forms.
+
+Standard C++ Named modules
+==
+
+This document was intended to be a manual first and foremost, however, we consider it helpful to
+introduce some language background here for readers who are not familiar with
+the new language feature. This document is not intended to be a language
+tutorial; it will only introduce necessary concepts about the
+structure and building of the project.
+
+Background and terminology
+--
+
+Modules
+~~~
+
+In this document, the term ``Modules``/``modules`` refers to standard C++ modules
+feature if it is not decorated by ``Clang``.
+
+Clang Modules
+~
+
+In this document, the term ``Clang Modules``/``Clang modules`` refer to Clang
+c++ modules extension. These are also known as ``Clang header modules``,
+``Clang module map modules`` or ``Clang c++ modules``.
+
+Module and module unit
+~~
+
+A module consists of one or more module units. A module unit is a special
+translation unit. Every module unit must have a module declaration. The syntax
+of the module declaration is:
+
+.. code-block:: c++
+
+  [export] module module_name[:partition_name];
+
+Terms enclosed in ``[]`` are optional. The syntax of ``module_name`` and ``partition_name``
+in regex form corresponds to ``[a-zA-Z_][a-zA-Z_0-9\.]*``. In particular, a literal dot ``.``
+in the name has no semantic meaning (e.g. implying a hierarchy).
+
+In this document, module units are classified into:
+
+* Primary module interface unit.
+
+* Module implementation unit.
+
+* Module interface partition unit.
+
+* Internal module partition unit.
+
+A primary module interface unit is a module unit whose module declaration is
+``export module module_name;``. The ``module_name`` here denotes the name of the
+module. A module should have one and only one primary module interface unit.
+
+A module implementation unit is a module unit whose module declaration is
+``module module_name;``. A module could have multiple module implementation
+units with the same declaration.
+
+A module interface partition unit is a module unit whose module declaration is
+``export module module_name:partition_name;``. The ``partition_name`` should be
+unique within any given module.
+
+An internal module partition unit is a module unit whose module declaration
+is ``module module_name:partition_name;``. The ``partition_name`` should be
+unique within any given module.
+
+In this document, we use the following umbrella terms:
+
+* A ``module interface unit`` refers to either a ``primary module interface unit``
+  or a ``module interface partition unit``.
+
+* An ``importable module unit`` refers to either a ``module interface unit``
+  or a ``internal module partition unit``.
+
+* A ``module partition unit`` refers to either a ``module interface partition unit``
+  or a ``internal module partition unit``.
+
+Built Module Interface file
+~~~

[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

There is another example we shouldn't make this specific for 
std::source_location::current(): 
https://github.com/llvm/llvm-project/issues/57459. I guess we can solve the 
issue too if we evaluate default argument at the caller position.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129488

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

[PATCH] D133085: [clang] trim trailing space in format tests. NFC

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added subscribers: aaron.ballman, ChuanqiXu.
ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

Do you have commit access? If you have, I remember LLVM encourages such fixes 
landed directly without review. (+ @aaron.ballman to make sure)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133085

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

[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D129488#3760982 , @ilya-biryukov 
wrote:

> @aaron.ballman, that's my reading of the standard as well. Do you think we 
> should proceed with the current approach or is there another direction worth 
> pursuing to make source_location work?
>
> In D129488#3760178 , @ChuanqiXu 
> wrote:
>
>> There is another example we shouldn't make this specific for 
>> std::source_location::current(): 
>> https://github.com/llvm/llvm-project/issues/57459. I guess we can solve the 
>> issue too if we evaluate default argument at the caller position.
>
> I think you're right and it would probably work automatically if we were to 
> recreate the default argument expression on every call where it appears.
> However, going this route for the particular purpose of checking module 
> visibility looks like an overkill. FWIW, see my attempt at this to fix 
> `source_location::current()` with immediate invocations D132941 
> .
> It's complicated, makes default arguments slower and encounters new failures 
> modes, e.g. there new errors with lambdas in default arguments.

It is just an example. I just wanted to say we could solve more problems if we 
chose that way. The issue of the module may not so painful and it shouldn't be 
hard to handle it specially. (Although specially handling looks not good, this 
is the reason why I didn't fix it). But after all, I am not objecting (nor 
approving) this for the modules example.

---

For this patch, I agree with @jyknight, it'd better to be constexpr instead of 
consteval. I feel it may be better to reflect this with wg21 instead of hacking 
in compilers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129488

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

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:2819-2823
+  // For two canonically equal types, return a type which has
+  // the common sugar between them. If Unqualified is true,
+  // both types need only be the same unqualified type.
+  // The result will drop the qualifiers which do not occur
+  // in both types.

For this comment, I want to know what if `X`and `Y` is not equal. Is there an 
assertion failure or special type returned?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283

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

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:1369
   }
+  QualType getDecayedType(QualType T, QualType Decayed) const;
 

Maybe we need a comment for this. The signature looks not straight forward and 
I can't relate this to the above comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130308

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

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

The summary looks good and I don't make a review in depth due to it is indeed 
large... (I'll see if I can take a deeper review)




Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:890-893
+  struct {
+bool DoIt;
+FunctionDecl *Existing = nullptr;
+  } Merge = {false};

Is this struct necessary? I feel two separate variables may be more clear.



Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:623-626
+  VisitDeclaratorDecl(D);
+  Record.AddDeclarationNameLoc(D->DNLoc, D->getDeclName());
+  Record.push_back(D->getIdentifierNamespace());
+

I still don't get the reason for the move. What's the benefit? Or why is it 
necessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:1369
   }
+  QualType getDecayedType(QualType T, QualType Decayed) const;
 

mizvekov wrote:
> ChuanqiXu wrote:
> > Maybe we need a comment for this. The signature looks not straight forward 
> > and I can't relate this to the above comment.
> I think I wanted to make this an internal variant, since we will probably 
> never need this outside of ASTContext anyway, but there is the friendship 
> situation between Type and ASTContext. Maybe making this private would be for 
> the best.
Yeah, the signature confused me as well... as long as the second parameter is 
`Decayed` already, why we need to get the decayed type again? I guess 
`Underlying` may be a better name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130308

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

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:623-626
+  VisitDeclaratorDecl(D);
+  Record.AddDeclarationNameLoc(D->DNLoc, D->getDeclName());
+  Record.push_back(D->getIdentifierNamespace());
+

mizvekov wrote:
> ChuanqiXu wrote:
> > I still don't get the reason for the move. What's the benefit? Or why is it 
> > necessary?
> Yeah, now the type can reference the template decl, so without moving this, 
> it can happen during import of the type that we try to read this function 
> template bits without having imported them yet.
Oh, I guess I met the problem before (D129748 ) and I made a workaround for it 
(https://reviews.llvm.org/D130331). If I understood right, the patch will solve 
that problem. I'll check it out later.

(This kind of code move looks dangerous you know and I'll take a double check)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:1369
   }
+  QualType getDecayedType(QualType T, QualType Decayed) const;
 

mizvekov wrote:
> ChuanqiXu wrote:
> > mizvekov wrote:
> > > ChuanqiXu wrote:
> > > > Maybe we need a comment for this. The signature looks not straight 
> > > > forward and I can't relate this to the above comment.
> > > I think I wanted to make this an internal variant, since we will probably 
> > > never need this outside of ASTContext anyway, but there is the friendship 
> > > situation between Type and ASTContext. Maybe making this private would be 
> > > for the best.
> > Yeah, the signature confused me as well... as long as the second parameter 
> > is `Decayed` already, why we need to get the decayed type again? I guess 
> > `Underlying` may be a better name.
> Ah I see, but this confusion already existed. See the definition of 
> DecayedType in Type.h, where you have the `QualType getDecayedType()` member. 
> This is what this Decayed parameter represents, what we will put in there to 
> be returned by that method.
> 
> For DecayedType / AdjustedType / AttributedType there exists this confusion 
> where they have two child nodes, which arbitrarily picking one of them to be 
> called the `UnderlyingType` could be controversial. You could make the case 
> that it should be the same one we use for implementing `desugar`, but there 
> are arguments there for the other case.
I see. I got your point. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130308

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

[PATCH] D133160: [cmake] Append CLANG_LIBDIR_SUFFIX to scan-build-py installation destination.

[Chuanqi Xu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1334e129a39c: [cmake] Append CLANG_LIBDIR_SUFFIX to 
scan-build-py installation destination (authored by sinan, committed by 
ChuanqiXu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133160

Files:
  clang/tools/scan-build-py/CMakeLists.txt


Index: clang/tools/scan-build-py/CMakeLists.txt
===
--- clang/tools/scan-build-py/CMakeLists.txt
+++ clang/tools/scan-build-py/CMakeLists.txt
@@ -88,7 +88,7 @@
  DEPENDS 
${CMAKE_CURRENT_SOURCE_DIR}/lib/libscanbuild/${lib})
   list(APPEND Depends ${CMAKE_BINARY_DIR}/lib/libscanbuild/${lib})
   install(PROGRAMS lib/libscanbuild/${lib}
-  DESTINATION lib/libscanbuild
+  DESTINATION lib${CLANG_LIBDIR_SUFFIX}/libscanbuild
   COMPONENT scan-build-py)
 endforeach()
 
@@ -106,7 +106,7 @@
  DEPENDS 
${CMAKE_CURRENT_SOURCE_DIR}/lib/libscanbuild/resources/${resource})
   list(APPEND Depends 
${CMAKE_BINARY_DIR}/lib/libscanbuild/resources/${resource})
   install(PROGRAMS lib/libscanbuild/resources/${resource}
-  DESTINATION lib/libscanbuild/resources
+  DESTINATION lib${CLANG_LIBDIR_SUFFIX}/libscanbuild/resources
   COMPONENT scan-build-py)
 endforeach()
 
@@ -122,7 +122,7 @@
  DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/lib/libear/${lib})
   list(APPEND Depends ${CMAKE_BINARY_DIR}/lib/libear/${lib})
   install(PROGRAMS lib/libear/${lib}
-  DESTINATION lib/libear
+  DESTINATION lib${CLANG_LIBDIR_SUFFIX}/libear
   COMPONENT scan-build-py)
 endforeach()
 


Index: clang/tools/scan-build-py/CMakeLists.txt
===
--- clang/tools/scan-build-py/CMakeLists.txt
+++ clang/tools/scan-build-py/CMakeLists.txt
@@ -88,7 +88,7 @@
  DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/lib/libscanbuild/${lib})
   list(APPEND Depends ${CMAKE_BINARY_DIR}/lib/libscanbuild/${lib})
   install(PROGRAMS lib/libscanbuild/${lib}
-  DESTINATION lib/libscanbuild
+  DESTINATION lib${CLANG_LIBDIR_SUFFIX}/libscanbuild
   COMPONENT scan-build-py)
 endforeach()
 
@@ -106,7 +106,7 @@
  DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/lib/libscanbuild/resources/${resource})
   list(APPEND Depends ${CMAKE_BINARY_DIR}/lib/libscanbuild/resources/${resource})
   install(PROGRAMS lib/libscanbuild/resources/${resource}
-  DESTINATION lib/libscanbuild/resources
+  DESTINATION lib${CLANG_LIBDIR_SUFFIX}/libscanbuild/resources
   COMPONENT scan-build-py)
 endforeach()
 
@@ -122,7 +122,7 @@
  DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/lib/libear/${lib})
   list(APPEND Depends ${CMAKE_BINARY_DIR}/lib/libear/${lib})
   install(PROGRAMS lib/libear/${lib}
-  DESTINATION lib/libear
+  DESTINATION lib${CLANG_LIBDIR_SUFFIX}/libear
   COMPONENT scan-build-py)
 endforeach()
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133341: [C++] [Coroutines] Prefer aligned (de)allocation for coroutines - implement the option2 of P2014R0

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu created this revision.
ChuanqiXu added reviewers: erichkeane, ychen, rjmccall.
ChuanqiXu added projects: clang, clang-language-wg.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.

This implements the option2 of 
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2014r0.pdf.

This also fixes https://github.com/llvm/llvm-project/issues/56671.

Although wg21 didn't get consensus for the direction of the problem, we're 
happy to have some implementation and user experience first. And from 
issue56671, the option2 should be the pursued one.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133341

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CodeGenCoroutines/coro-aligned-alloc-2.cpp
  clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
  clang/test/SemaCXX/coroutine-alloc-4.cpp

Index: clang/test/SemaCXX/coroutine-alloc-4.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/coroutine-alloc-4.cpp
@@ -0,0 +1,116 @@
+// Tests that we'll find aligned allocation funciton properly.
+// RUN: %clang_cc1 %s -std=c++20 %s -fsyntax-only -verify -fcoro-aligned-allocation
+
+#include "Inputs/std-coroutine.h"
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+enum class align_val_t : size_t {};
+}
+
+struct task {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t); // expected-warning 1+{{found non aligned allocation function for coroutine}}
+  };
+};
+
+task f() {   // expected-error {{'operator new' provided by 'std::coroutine_traits::promise_type' (aka 'task::promise_type') is not usable}}
+co_return 43;
+}
+
+struct task2 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task2{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t);
+  };
+};
+
+// no diagnostic expected
+task2 f1() {
+co_return 43;
+}
+
+struct task3 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task3{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t) noexcept;
+void *operator new(std::size_t) noexcept;
+static auto get_return_object_on_allocation_failure() { return task3{}; }
+  };
+};
+
+// no diagnostic expected
+task3 f2() {
+co_return 43;
+}
+
+struct task4 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task4{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t, int, double, int) noexcept;
+  };
+};
+
+// no diagnostic expected
+task4 f3(int, double, int) {
+co_return 43;
+}
+
+struct task5 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task5{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+  };
+};
+
+// no diagnostic expected.
+// The aligned allocation will be declared by the compiler.
+task5 f4() {
+co_return 43;
+}
+
+namespace std {
+  struct nothrow_t {};
+  constexpr nothrow_t nothrow = {};
+}
+
+struct task6 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task6{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+static task6 get_return_object_on_allocation_failure() { return task6{}; }
+  };
+};
+
+task6 f5() { // expected-error 1+{{unable to find '::operator new(size_t, align_val_t, nothrow_t)' for 'f5'}}
+co_return 43;
+}
+
+void *operator new(std::size_t, std::align_val_t, std::nothrow_t) noexcept; 
+
+task6 f6() {
+ 

[PATCH] D133341: [C++] [Coroutines] Prefer aligned (de)allocation for coroutines - implement the option2 of P2014R0

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 458116.
ChuanqiXu added a comment.

Remove invisible character... there are always invisible characters in the 
paper..


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

https://reviews.llvm.org/D133341

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CodeGenCoroutines/coro-aligned-alloc-2.cpp
  clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
  clang/test/SemaCXX/coroutine-alloc-4.cpp

Index: clang/test/SemaCXX/coroutine-alloc-4.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/coroutine-alloc-4.cpp
@@ -0,0 +1,116 @@
+// Tests that we'll find aligned allocation funciton properly.
+// RUN: %clang_cc1 %s -std=c++20 %s -fsyntax-only -verify -fcoro-aligned-allocation
+
+#include "Inputs/std-coroutine.h"
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+enum class align_val_t : size_t {};
+}
+
+struct task {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t); // expected-warning 1+{{found non aligned allocation function for coroutine}}
+  };
+};
+
+task f() {   // expected-error {{'operator new' provided by 'std::coroutine_traits::promise_type' (aka 'task::promise_type') is not usable}}
+co_return 43;
+}
+
+struct task2 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task2{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t);
+  };
+};
+
+// no diagnostic expected
+task2 f1() {
+co_return 43;
+}
+
+struct task3 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task3{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t) noexcept;
+void *operator new(std::size_t) noexcept;
+static auto get_return_object_on_allocation_failure() { return task3{}; }
+  };
+};
+
+// no diagnostic expected
+task3 f2() {
+co_return 43;
+}
+
+struct task4 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task4{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t, int, double, int) noexcept;
+  };
+};
+
+// no diagnostic expected
+task4 f3(int, double, int) {
+co_return 43;
+}
+
+struct task5 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task5{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+  };
+};
+
+// no diagnostic expected.
+// The aligned allocation will be declared by the compiler.
+task5 f4() {
+co_return 43;
+}
+
+namespace std {
+  struct nothrow_t {};
+  constexpr nothrow_t nothrow = {};
+}
+
+struct task6 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task6{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+static task6 get_return_object_on_allocation_failure() { return task6{}; }
+  };
+};
+
+task6 f5() { // expected-error 1+{{unable to find '::operator new(size_t, align_val_t, nothrow_t)' for 'f5'}}
+co_return 43;
+}
+
+void *operator new(std::size_t, std::align_val_t, std::nothrow_t) noexcept; 
+
+task6 f6() {
+co_return 43;
+}
Index: clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
@@ -0,0 +1,105 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 \
+// RUN:   -fcoro-aligned-allocation -S -emit-llvm %s -o - -disable-llvm-passes \
+// RUN:   | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+

[PATCH] D133341: [C++] [Coroutines] Prefer aligned (de)allocation for coroutines - implement the option2 of P2014R0

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 458353.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D133341

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CodeGenCoroutines/coro-aligned-alloc-2.cpp
  clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
  clang/test/SemaCXX/coroutine-alloc-4.cpp

Index: clang/test/SemaCXX/coroutine-alloc-4.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/coroutine-alloc-4.cpp
@@ -0,0 +1,116 @@
+// Tests that we'll find aligned allocation funciton properly.
+// RUN: %clang_cc1 %s -std=c++20 %s -fsyntax-only -verify -fcoro-aligned-allocation
+
+#include "Inputs/std-coroutine.h"
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+enum class align_val_t : size_t {};
+}
+
+struct task {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t); // expected-warning 1+{{found non aligned allocation function for coroutine}}
+  };
+};
+
+task f() {   // expected-error {{'operator new' provided by 'std::coroutine_traits::promise_type' (aka 'task::promise_type') is not usable}}
+co_return 43;
+}
+
+struct task2 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task2{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t);
+  };
+};
+
+// no diagnostic expected
+task2 f1() {
+co_return 43;
+}
+
+struct task3 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task3{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t) noexcept;
+void *operator new(std::size_t) noexcept;
+static auto get_return_object_on_allocation_failure() { return task3{}; }
+  };
+};
+
+// no diagnostic expected
+task3 f2() {
+co_return 43;
+}
+
+struct task4 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task4{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t, int, double, int) noexcept;
+  };
+};
+
+// no diagnostic expected
+task4 f3(int, double, int) {
+co_return 43;
+}
+
+struct task5 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task5{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+  };
+};
+
+// no diagnostic expected.
+// The aligned allocation will be declared by the compiler.
+task5 f4() {
+co_return 43;
+}
+
+namespace std {
+  struct nothrow_t {};
+  constexpr nothrow_t nothrow = {};
+}
+
+struct task6 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task6{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+static task6 get_return_object_on_allocation_failure() { return task6{}; }
+  };
+};
+
+task6 f5() { // expected-error 1+{{unable to find '::operator new(size_t, align_val_t, nothrow_t)' for 'f5'}}
+co_return 43;
+}
+
+void *operator new(std::size_t, std::align_val_t, std::nothrow_t) noexcept; 
+
+task6 f6() {
+co_return 43;
+}
Index: clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
@@ -0,0 +1,105 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 \
+// RUN:   -fcoro-aligned-allocation -S -emit-llvm %s -o - -disable-llvm-passes \
+// RUN:   | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+enum class

[PATCH] D133341: [C++] [Coroutines] Prefer aligned (de)allocation for coroutines - implement the option2 of P2014R0

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked 2 inline comments as done.
ChuanqiXu added inline comments.



Comment at: clang/lib/CodeGen/CGCoroutine.cpp:688
+auto &Context = getContext();
+auto SizeTy = Context.getSizeType();
+auto T = Builder.getIntNTy(Context.getTypeSize(SizeTy));

erichkeane wrote:
> Don't use 'auto' unless the RHS has the type in it clearly.  SO this should 
> be QualType I think?  I also see you've broken that coding standard rule 
> above.
Oh, sorry. I did move the code simply and didn't double check it. I've handled 
it in an separate NFC patch: 
https://github.com/llvm/llvm-project/commit/5f571eeb3f764c6d97b81822464ea420adef2cf7


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

https://reviews.llvm.org/D133341

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

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:623-626
+  VisitDeclaratorDecl(D);
+  Record.AddDeclarationNameLoc(D->DNLoc, D->getDeclName());
+  Record.push_back(D->getIdentifierNamespace());
+

ChuanqiXu wrote:
> mizvekov wrote:
> > ChuanqiXu wrote:
> > > I still don't get the reason for the move. What's the benefit? Or why is 
> > > it necessary?
> > Yeah, now the type can reference the template decl, so without moving this, 
> > it can happen during import of the type that we try to read this function 
> > template bits without having imported them yet.
> Oh, I guess I met the problem before (D129748 ) and I made a workaround for 
> it (https://reviews.llvm.org/D130331). If I understood right, the patch will 
> solve that problem. I'll check it out later.
> 
> (This kind of code move looks dangerous you know and I'll take a double check)
After looking into the detailed change for the serialization part, I feel it is 
a not-so-good workaround indeed.. It looks like we need a better method to 
delay reading the type in the serializer. And I'm looking at it. @mizvekov 
would you like to rebase the series of patches to the main branch so that I can 
test it actually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:623-626
+  VisitDeclaratorDecl(D);
+  Record.AddDeclarationNameLoc(D->DNLoc, D->getDeclName());
+  Record.push_back(D->getIdentifierNamespace());
+

ChuanqiXu wrote:
> ChuanqiXu wrote:
> > mizvekov wrote:
> > > ChuanqiXu wrote:
> > > > I still don't get the reason for the move. What's the benefit? Or why 
> > > > is it necessary?
> > > Yeah, now the type can reference the template decl, so without moving 
> > > this, it can happen during import of the type that we try to read this 
> > > function template bits without having imported them yet.
> > Oh, I guess I met the problem before (D129748 ) and I made a workaround for 
> > it (https://reviews.llvm.org/D130331). If I understood right, the patch 
> > will solve that problem. I'll check it out later.
> > 
> > (This kind of code move looks dangerous you know and I'll take a double 
> > check)
> After looking into the detailed change for the serialization part, I feel it 
> is a not-so-good workaround indeed.. It looks like we need a better method to 
> delay reading the type in the serializer. And I'm looking at it. @mizvekov 
> would you like to rebase the series of patches to the main branch so that I 
> can test it actually.
Or would it be simpler to rebase and squash them into a draft revision?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:623-626
+  VisitDeclaratorDecl(D);
+  Record.AddDeclarationNameLoc(D->DNLoc, D->getDeclName());
+  Record.push_back(D->getIdentifierNamespace());
+

mizvekov wrote:
> ChuanqiXu wrote:
> > ChuanqiXu wrote:
> > > ChuanqiXu wrote:
> > > > mizvekov wrote:
> > > > > ChuanqiXu wrote:
> > > > > > I still don't get the reason for the move. What's the benefit? Or 
> > > > > > why is it necessary?
> > > > > Yeah, now the type can reference the template decl, so without moving 
> > > > > this, it can happen during import of the type that we try to read 
> > > > > this function template bits without having imported them yet.
> > > > Oh, I guess I met the problem before (D129748 ) and I made a workaround 
> > > > for it (https://reviews.llvm.org/D130331). If I understood right, the 
> > > > patch will solve that problem. I'll check it out later.
> > > > 
> > > > (This kind of code move looks dangerous you know and I'll take a double 
> > > > check)
> > > After looking into the detailed change for the serialization part, I feel 
> > > it is a not-so-good workaround indeed.. It looks like we need a better 
> > > method to delay reading the type in the serializer. And I'm looking at 
> > > it. @mizvekov would you like to rebase the series of patches to the main 
> > > branch so that I can test it actually.
> > Or would it be simpler to rebase and squash them into a draft revision?
> I had given this some thought, and it made sense to me that we should deal 
> with the template bits first, since these are closer to the introducer for 
> these declarations, and so that it would be harder to have a dependence the 
> other way around.
> 
> But I would like to hear your thoughts on this after you have taken a better 
> look.
> I am working on a bunch of things right now, I should be able to rebase this 
> on the next few days, but otherwise
> I last rebased about 4 days ago, so you can also check that out at 
> https://github.com/mizvekov/llvm-project/tree/resugar
> That link has the whole stack, you probably should check out just the commit 
> for this patch, as you are probably going to encounter issues with the 
> resugarer if you try it on substantial code bases.
> It will carry other changes with it, but I think those should be safe.
I won't say it is bad to deal with template bits first. I just feel it is a 
workaround to avoid the circular dependent problem in deserialization. Or in 
another word, here the method works due to you put some decls* in the template 
parameter types. And we avoid the circular dependent problem by adjusting the 
order we deserializes. The reasons why I don't feel it is good include:
(1) Although we touched template function decl and template var decl, we don't 
touched template var decl. I guess it'll be a problem now or later.
(2) The solution here can't solve the similar circular dependent problem I 
sawed in attributes. So the method only workarounds some resulting of the same 
problem.

Or in one shorter explanation, it should be greater to solve the root problems. 
I have an idea and I am going to to do a proof-of-concept implementation first 
since I feel like nobody are happy about an unimplementable idea. Generally I 
don't like to block patches due to such reasons since it is completely not your 
fault but I guess it may be better to wait some time. Since if we want to allow 
workarounds first and clear the workarounds, things will be harder. If you want 
a timeline, I guess 2 months may be reasonable choices. I mean if I can't make 
it in 2 months and other reviewers feel this is good (what I am seeing), I feel 
bad to block this. (But if we're more patient, it'll be better). How do you 
think about this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:623-626
+  VisitDeclaratorDecl(D);
+  Record.AddDeclarationNameLoc(D->DNLoc, D->getDeclName());
+  Record.push_back(D->getIdentifierNamespace());
+

mizvekov wrote:
> ChuanqiXu wrote:
> > mizvekov wrote:
> > > ChuanqiXu wrote:
> > > > ChuanqiXu wrote:
> > > > > ChuanqiXu wrote:
> > > > > > mizvekov wrote:
> > > > > > > ChuanqiXu wrote:
> > > > > > > > I still don't get the reason for the move. What's the benefit? 
> > > > > > > > Or why is it necessary?
> > > > > > > Yeah, now the type can reference the template decl, so without 
> > > > > > > moving this, it can happen during import of the type that we try 
> > > > > > > to read this function template bits without having imported them 
> > > > > > > yet.
> > > > > > Oh, I guess I met the problem before (D129748 ) and I made a 
> > > > > > workaround for it (https://reviews.llvm.org/D130331). If I 
> > > > > > understood right, the patch will solve that problem. I'll check it 
> > > > > > out later.
> > > > > > 
> > > > > > (This kind of code move looks dangerous you know and I'll take a 
> > > > > > double check)
> > > > > After looking into the detailed change for the serialization part, I 
> > > > > feel it is a not-so-good workaround indeed.. It looks like we need a 
> > > > > better method to delay reading the type in the serializer. And I'm 
> > > > > looking at it. @mizvekov would you like to rebase the series of 
> > > > > patches to the main branch so that I can test it actually.
> > > > Or would it be simpler to rebase and squash them into a draft revision?
> > > I had given this some thought, and it made sense to me that we should 
> > > deal with the template bits first, since these are closer to the 
> > > introducer for these declarations, and so that it would be harder to have 
> > > a dependence the other way around.
> > > 
> > > But I would like to hear your thoughts on this after you have taken a 
> > > better look.
> > > I am working on a bunch of things right now, I should be able to rebase 
> > > this on the next few days, but otherwise
> > > I last rebased about 4 days ago, so you can also check that out at 
> > > https://github.com/mizvekov/llvm-project/tree/resugar
> > > That link has the whole stack, you probably should check out just the 
> > > commit for this patch, as you are probably going to encounter issues with 
> > > the resugarer if you try it on substantial code bases.
> > > It will carry other changes with it, but I think those should be safe.
> > I won't say it is bad to deal with template bits first. I just feel it is a 
> > workaround to avoid the circular dependent problem in deserialization. Or 
> > in another word, here the method works due to you put some decls* in the 
> > template parameter types. And we avoid the circular dependent problem by 
> > adjusting the order we deserializes. The reasons why I don't feel it is 
> > good include:
> > (1) Although we touched template function decl and template var decl, we 
> > don't touched template var decl. I guess it'll be a problem now or later.
> > (2) The solution here can't solve the similar circular dependent problem I 
> > sawed in attributes. So the method only workarounds some resulting of the 
> > same problem.
> > 
> > Or in one shorter explanation, it should be greater to solve the root 
> > problems. I have an idea and I am going to to do a proof-of-concept 
> > implementation first since I feel like nobody are happy about an 
> > unimplementable idea. Generally I don't like to block patches due to such 
> > reasons since it is completely not your fault but I guess it may be better 
> > to wait some time. Since if we want to allow workarounds first and clear 
> > the workarounds, things will be harder. If you want a timeline, I guess 2 
> > months may be reasonable choices. I mean if I can't make it in 2 months and 
> > other reviewers feel this is good (what I am seeing), I feel bad to block 
> > this. (But if we're more patient, it'll be better). How do you think about 
> > this?
> Well we touch FunctionTemplates and VariableTemplates in this patch, because 
> they were not doing template first.
> For whatever reason, class templates were already doing template first, so no 
> need to fix that.
> 
> So this patch at least puts that into consistency.
> 
> Also, this patch is a pre-requisite for the template resugaring 
> specialization project I am working on, and the deadline for the whole 
> project is about two months from now.
> 
> If I leave merging this patch until the end, it seems impossible that I will 
> finish in time, as we will leave field testing this to the very end.
> 
> So while I understand the need for a better approach, it is indeed putting me 
> in an impossible situation.
> Also, this patch is a pre-requisite for the template resugaring 
> specialization project I am working on, and the deadline for the whole 
> p

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:623-626
+  VisitDeclaratorDecl(D);
+  Record.AddDeclarationNameLoc(D->DNLoc, D->getDeclName());
+  Record.push_back(D->getIdentifierNamespace());
+

mizvekov wrote:
> ChuanqiXu wrote:
> > mizvekov wrote:
> > > ChuanqiXu wrote:
> > > > mizvekov wrote:
> > > > > ChuanqiXu wrote:
> > > > > > ChuanqiXu wrote:
> > > > > > > ChuanqiXu wrote:
> > > > > > > > mizvekov wrote:
> > > > > > > > > ChuanqiXu wrote:
> > > > > > > > > > I still don't get the reason for the move. What's the 
> > > > > > > > > > benefit? Or why is it necessary?
> > > > > > > > > Yeah, now the type can reference the template decl, so 
> > > > > > > > > without moving this, it can happen during import of the type 
> > > > > > > > > that we try to read this function template bits without 
> > > > > > > > > having imported them yet.
> > > > > > > > Oh, I guess I met the problem before (D129748 ) and I made a 
> > > > > > > > workaround for it (https://reviews.llvm.org/D130331). If I 
> > > > > > > > understood right, the patch will solve that problem. I'll check 
> > > > > > > > it out later.
> > > > > > > > 
> > > > > > > > (This kind of code move looks dangerous you know and I'll take 
> > > > > > > > a double check)
> > > > > > > After looking into the detailed change for the serialization 
> > > > > > > part, I feel it is a not-so-good workaround indeed.. It looks 
> > > > > > > like we need a better method to delay reading the type in the 
> > > > > > > serializer. And I'm looking at it. @mizvekov would you like to 
> > > > > > > rebase the series of patches to the main branch so that I can 
> > > > > > > test it actually.
> > > > > > Or would it be simpler to rebase and squash them into a draft 
> > > > > > revision?
> > > > > I had given this some thought, and it made sense to me that we should 
> > > > > deal with the template bits first, since these are closer to the 
> > > > > introducer for these declarations, and so that it would be harder to 
> > > > > have a dependence the other way around.
> > > > > 
> > > > > But I would like to hear your thoughts on this after you have taken a 
> > > > > better look.
> > > > > I am working on a bunch of things right now, I should be able to 
> > > > > rebase this on the next few days, but otherwise
> > > > > I last rebased about 4 days ago, so you can also check that out at 
> > > > > https://github.com/mizvekov/llvm-project/tree/resugar
> > > > > That link has the whole stack, you probably should check out just the 
> > > > > commit for this patch, as you are probably going to encounter issues 
> > > > > with the resugarer if you try it on substantial code bases.
> > > > > It will carry other changes with it, but I think those should be safe.
> > > > I won't say it is bad to deal with template bits first. I just feel it 
> > > > is a workaround to avoid the circular dependent problem in 
> > > > deserialization. Or in another word, here the method works due to you 
> > > > put some decls* in the template parameter types. And we avoid the 
> > > > circular dependent problem by adjusting the order we deserializes. The 
> > > > reasons why I don't feel it is good include:
> > > > (1) Although we touched template function decl and template var decl, 
> > > > we don't touched template var decl. I guess it'll be a problem now or 
> > > > later.
> > > > (2) The solution here can't solve the similar circular dependent 
> > > > problem I sawed in attributes. So the method only workarounds some 
> > > > resulting of the same problem.
> > > > 
> > > > Or in one shorter explanation, it should be greater to solve the root 
> > > > problems. I have an idea and I am going to to do a proof-of-concept 
> > > > implementation first since I feel like nobody are happy about an 
> > > > unimplementable idea. Generally I don't like to block patches due to 
> > > > such reasons since it is completely not your fault but I guess it may 
> > > > be better to wait some time. Since if we want to allow workarounds 
> > > > first and clear the workarounds, things will be harder. If you want a 
> > > > timeline, I guess 2 months may be reasonable choices. I mean if I can't 
> > > > make it in 2 months and other reviewers feel this is good (what I am 
> > > > seeing), I feel bad to block this. (But if we're more patient, it'll be 
> > > > better). How do you think about this?
> > > Well we touch FunctionTemplates and VariableTemplates in this patch, 
> > > because they were not doing template first.
> > > For whatever reason, class templates were already doing template first, 
> > > so no need to fix that.
> > > 
> > > So this patch at least puts that into consistency.
> > > 
> > > Also, this patch is a pre-requisite for the template resugaring 
> > > specialization project I am working on, and the deadline for the whole 
> > > project is about two months from now.
> > > 
> > > If I leave merging this pat

[PATCH] D133341: [C++] [Coroutines] Prefer aligned (de)allocation for coroutines - implement the option2 of P2014R0

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked an inline comment as done.
ChuanqiXu added a comment.

@ychen @rjmccall ping~


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

https://reviews.llvm.org/D133341

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

[PATCH] D132352: Introduce noread_thread_id to address the thread identification problem in coroutines

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

@nhaehnle @jyknight @nikic @efriedma @fhahn ping~


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

https://reviews.llvm.org/D132352

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

[PATCH] D133341: [C++] [Coroutines] Prefer aligned (de)allocation for coroutines - implement the option2 of P2014R0

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 459997.
ChuanqiXu marked 5 inline comments as done.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D133341

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CodeGenCoroutines/coro-aligned-alloc-2.cpp
  clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
  clang/test/SemaCXX/coroutine-alloc-4.cpp

Index: clang/test/SemaCXX/coroutine-alloc-4.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/coroutine-alloc-4.cpp
@@ -0,0 +1,116 @@
+// Tests that we'll find aligned allocation funciton properly.
+// RUN: %clang_cc1 %s -std=c++20 %s -fsyntax-only -verify -fcoro-aligned-allocation
+
+#include "Inputs/std-coroutine.h"
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+enum class align_val_t : size_t {};
+}
+
+struct task {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t); // expected-warning 1+{{found non aligned allocation function for coroutine}}
+  };
+};
+
+task f() {
+co_return 43;
+}
+
+struct task2 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task2{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t);
+  };
+};
+
+// no diagnostic expected
+task2 f1() {
+co_return 43;
+}
+
+struct task3 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task3{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t) noexcept;
+void *operator new(std::size_t) noexcept;
+static auto get_return_object_on_allocation_failure() { return task3{}; }
+  };
+};
+
+// no diagnostic expected
+task3 f2() {
+co_return 43;
+}
+
+struct task4 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task4{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t, int, double, int) noexcept;
+  };
+};
+
+// no diagnostic expected
+task4 f3(int, double, int) {
+co_return 43;
+}
+
+struct task5 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task5{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+  };
+};
+
+// no diagnostic expected.
+// The aligned allocation will be declared by the compiler.
+task5 f4() {
+co_return 43;
+}
+
+namespace std {
+  struct nothrow_t {};
+  constexpr nothrow_t nothrow = {};
+}
+
+struct task6 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task6{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+static task6 get_return_object_on_allocation_failure() { return task6{}; }
+  };
+};
+
+task6 f5() { // expected-error 1+{{unable to find '::operator new(size_t, align_val_t, nothrow_t)' for 'f5'}}
+co_return 43;
+}
+
+void *operator new(std::size_t, std::align_val_t, std::nothrow_t) noexcept; 
+
+task6 f6() {
+co_return 43;
+}
Index: clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
@@ -0,0 +1,190 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 \
+// RUN:   -fcoro-aligned-allocation -S -emit-llvm %s -o - -disable-llvm-passes \
+// RUN:   | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+enum class align_val_t : size_t {};
+}
+
+struct task {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}

[PATCH] D133341: [C++] [Coroutines] Prefer aligned (de)allocation for coroutines - implement the option2 of P2014R0

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked 2 inline comments as done.
ChuanqiXu added a comment.

In D133341#3788283 , @ychen wrote:

> It surprises me that Option 2 does not change 
> https://eel.is/c++draft/dcl.fct.def.coroutine#10. For consistency, I think it 
> should. And according to your test case, it deals with alignment as expected. 
> Probably we should change the P2014R0 wording accordingly. Before that, let's 
> just mention this difference in the Clang release notes.

Yeah, in fact due to the wording of coroutine allocation has changed slightly 
recently, the wording of P2014  must be updated 
before merged. And I agree it should be consistent.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11283
+def err_conflicting_aligned_options : Error <
+  "conflicting option '-fcoro-aligned-allocation' and 
'-fno-aligned-allocation'"
 >;

ychen wrote:
> Since we're digressing from the usual operator new/delete look-up rules per 
> Option 2, I think it might be easier to just *not* respect 
> `-fno-aligned-allocation` and `-fno-sized-deallocation`. Using 
> '-fcoro-aligned-allocation' explicitly should permit us to assume these 
> functions could be found.
I guess it may not be good to enable `-fsized-deallocation` automatically if 
`-fcoro-aligned-allocation` enabled. Since it looks like there are other 
reasons why we disabled `-fsized-deallocation` before. And it looks it will 
block our users to use `-fcoro-aligned-allocation`. For the 
`-faligned-allocation` flag, this is enabled by default but people could 
disable it explicitly. So the check here is for that.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:1302-1318
   // According to [dcl.fct.def.coroutine]p9, Lookup allocation functions using 
a
   // parameter list composed of the requested size of the coroutine state being
   // allocated, followed by the coroutine function's arguments. If a matching
   // allocation function exists, use it. Otherwise, use an allocation function
   // that just takes the requested size.
   //
   // [dcl.fct.def.coroutine]p9

ychen wrote:
> Update comment here.
Since P2014 is not standardized, I feel it might not be good to edit them here.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:1400
 
+  // If we found a non-aligned allocation function in the promise_type,
+  // it indicates the user forgot to update the allocation function. Let's emit

ychen wrote:
> Option 2's order of look-up is 
> ```
> void* T::operator new  ( std::size_t count, std::align_val_t al, 
> user-defined-args... );
> void* T::operator new  ( std::size_t count, std::align_val_t al);
> void* T::operator new  ( std::size_t count, user-defined-args... );
> void* T::operator new  ( std::size_t count);
> void* operator new  ( std::size_t count, std::align_val_t al );
> ```
> Why not allow `void* T::operator new  ( std::size_t count);` here?
My original thought is to be as strict as we can. But now I feel a warning may 
be good enough.



Comment at: clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp:94
+void return_value(int) {}
+void operator delete(void *ptr, std::align_val_t);
+  };

ychen wrote:
> Please add test cases for 
> ```
> void operator delete  ( void* ptr, std::size_t, std::align_val_t);
> void operator delete  ( void* ptr, std::size_t);
> void operator delete  ( void* ptr);
> void ::operator delete  ( void* ptr, std::size_t, std::align_val_t);
> void ::operator delete  ( void* ptr, std::size_t);
> void ::operator delete  ( void* ptr);
> ```
> in that lookup order, and makes sure the look-up order is expected.
I've tried my best. But it looks hard to test these operator delete under 
global namespace since they are automatically declared by the compiler.



Comment at: clang/test/SemaCXX/coroutine-alloc-4.cpp:49
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t) noexcept;
+void *operator new(std::size_t) noexcept;

ychen wrote:
> Like this test case, please add additional test cases to check the expected 
> look-up order, one test for each consecutive pair should be good.
> 
> ```
> void* T::operator new  ( std::size_t count, std::align_val_t al, 
> user-defined-args... );
> void* T::operator new  ( std::size_t count, std::align_val_t al);
> void* T::operator new  ( std::size_t count, user-defined-args... );
> void* T::operator new  ( std::size_t count);
> void* operator new  ( std::size_t count, std::align_val_t al );
> ```
> 
> 
Yeah, I'm testing this in CodeGenCoroutines. (It is hard to test the selection 
in Sema Test)


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

https://reviews.llvm.org/D133341

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

[PATCH] D133341: [C++] [Coroutines] Prefer aligned (de)allocation for coroutines - implement the option2 of P2014R0

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 460315.
ChuanqiXu marked 6 inline comments as done.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D133341

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CodeGenCoroutines/coro-aligned-alloc-2.cpp
  clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
  clang/test/SemaCXX/coroutine-alloc-4.cpp

Index: clang/test/SemaCXX/coroutine-alloc-4.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/coroutine-alloc-4.cpp
@@ -0,0 +1,116 @@
+// Tests that we'll find aligned allocation funciton properly.
+// RUN: %clang_cc1 %s -std=c++20 %s -fsyntax-only -verify -fcoro-aligned-allocation
+
+#include "Inputs/std-coroutine.h"
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+enum class align_val_t : size_t {};
+}
+
+struct task {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t); // expected-warning 1+{{under -fcoro-aligned-allocation, the non-aligned allocation function for the promise type 'f' has higher precedence than the global aligned allocation function}}
+  };
+};
+
+task f() {
+co_return 43;
+}
+
+struct task2 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task2{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t);
+  };
+};
+
+// no diagnostic expected
+task2 f1() {
+co_return 43;
+}
+
+struct task3 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task3{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t) noexcept;
+void *operator new(std::size_t) noexcept;
+static auto get_return_object_on_allocation_failure() { return task3{}; }
+  };
+};
+
+// no diagnostic expected
+task3 f2() {
+co_return 43;
+}
+
+struct task4 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task4{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t, int, double, int) noexcept;
+  };
+};
+
+// no diagnostic expected
+task4 f3(int, double, int) {
+co_return 43;
+}
+
+struct task5 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task5{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+  };
+};
+
+// no diagnostic expected.
+// The aligned allocation will be declared by the compiler.
+task5 f4() {
+co_return 43;
+}
+
+namespace std {
+  struct nothrow_t {};
+  constexpr nothrow_t nothrow = {};
+}
+
+struct task6 {
+  struct promise_type {
+auto initial_suspend() { return std::suspend_always{}; }
+auto final_suspend() noexcept { return std::suspend_always{}; }
+auto get_return_object() { return task6{}; }
+void unhandled_exception() {}
+void return_value(int) {}
+static task6 get_return_object_on_allocation_failure() { return task6{}; }
+  };
+};
+
+task6 f5() { // expected-error 1+{{unable to find '::operator new(size_t, align_val_t, nothrow_t)' for 'f5'}}
+co_return 43;
+}
+
+void *operator new(std::size_t, std::align_val_t, std::nothrow_t) noexcept; 
+
+task6 f6() {
+co_return 43;
+}
Index: clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp
@@ -0,0 +1,190 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 \
+// RUN:   -fcoro-aligned-allocation -S -emit-llvm %s -o - -disable-llvm-passes \
+// RUN:   | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+enum class align_val_t : size_t

[PATCH] D133341: [C++] [Coroutines] Prefer aligned (de)allocation for coroutines - implement the option2 of P2014R0

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked 2 inline comments as done.
ChuanqiXu added inline comments.



Comment at: clang/test/SemaCXX/coroutine-alloc-4.cpp:49
+void return_value(int) {}
+void *operator new(std::size_t, std::align_val_t) noexcept;
+void *operator new(std::size_t) noexcept;

ychen wrote:
> ChuanqiXu wrote:
> > ychen wrote:
> > > Like this test case, please add additional test cases to check the 
> > > expected look-up order, one test for each consecutive pair should be good.
> > > 
> > > ```
> > > void* T::operator new  ( std::size_t count, std::align_val_t al, 
> > > user-defined-args... );
> > > void* T::operator new  ( std::size_t count, std::align_val_t al);
> > > void* T::operator new  ( std::size_t count, user-defined-args... );
> > > void* T::operator new  ( std::size_t count);
> > > void* operator new  ( std::size_t count, std::align_val_t al );
> > > ```
> > > 
> > > 
> > Yeah, I'm testing this in CodeGenCoroutines. (It is hard to test the 
> > selection in Sema Test)
> Thanks for adding the overload. I think the `noexcept` on operator new is not 
> necessary. Strictly speaking, it is not a conforming API.
The noexcept here is necessary. The specs says the allocation function should 
have a noexcept specifier if get_return_object_on_allocation_failure presents.


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

https://reviews.llvm.org/D133341

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

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> We need to mention this in the docs and the ReleaseNotes since we add a new 
> flag.



> To make this self contained, we need to add a check in the frontend and emit 
> errors if the compiler find the throwing exception's destructor may throw. 
> This is not required in the current patch but it may be good to add a FIXME 
> or TODO somewhere.

There are 2 comments not addressed yet : )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108905

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

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

Oh, I am not saying the legacy and old comment. I mean you need to touch 
ReleaseNotes.rst and UserManual.rst since we add a new flag. Also we need 
either add a TODO/FIXME saying we need to emit an error in Sema if we find the 
the dtor of the exception we're throwing may throw or implement the semantics 
actually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108905

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

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/docs/UsersManual.rst:2145-2147
+   ``catch (...) { ... }``. This option tells Clang that an exception object's
+   destructor does not throw, even if the destructor is annotated as
+   ``noexcept(false)``.

I think it is better to treat the program as invalid if the compiler find the 
exception object's destructor is ``noexcept(false)``. The earlier error message 
is better than debugging and understanding what happened actually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108905

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

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D108905#4655907 , @MaskRay wrote:

> In D108905#4655694 , @ChuanqiXu 
> wrote:
>
>> Oh, I am not saying the legacy and old comment. I mean you need to touch 
>> ReleaseNotes.rst and UserManual.rst since we add a new flag. Also we need 
>> either add a TODO/FIXME saying we need to emit an error in Sema if we find 
>> the the dtor of the exception we're throwing may throw or implement the 
>> semantics actually.
>
> Thanks for the reminder about `ReleaseNotes.rst` and `UsersManual.rst`!
>
> I think many changes don't update `UsersManual.rst` but this option is 
> probably quite useful and therefore deserves an entry. Added.
> The primary change is to `clang/lib/CodeGen/ItaniumCXXABI.cpp`, which does 
> not report warnings.
> If we want to implement a warning, we should probably do it in 
> clang/lib/Sema/SemaDeclCXX.cpp `Sema::BuildExceptionDeclaration`, which is 
> not touched in this file, so a TODO seems not appropriate...
>
> Is the warning to warn about `noexcept(false)` destructor in an 
> exception-declaration ? When?
> If at catch handlers, unfortunately we are cannot determine the exception 
> object for a `catch (...) { ... }` (used by coroutines).
> Technically, even if a destructor is `noexcept(false)`, the destructor may 
> not throw when `__cxa_end_catch` destroys the object.
>
> So we probably should warn about throw expressions, which can be done in 
> `Sema::CheckCXXThrowOperand` and I will investigate it.
> However, this warning appears orthogonal to `-fassume-nothrow-exception-dtor`.
> We can argue that even without `-fassume-nothrow-exception-dtor`, an thrown 
> object with a `noexcept(false)` destructor is probably not a good idea.
> However, I am on the fence whether the warning should be enabled-by-default.

The idea of diagnosing such cases is originally raised by @rjmccall  in 
https://github.com/llvm/llvm-project/issues/57375#issuecomment-1230590204. And 
in fact,  John is suggesting an error instead of a warning. To be honest, in 
our downstream implementation, we didn't implement that semantical check 
neither.

For the implementation, I think `Sema::CheckCXXThrowOperand` may be a good 
place and we can left a TODO there.

Technically, we're using a dialect after we enable 
`-fassume-nothrow-exception-dtor`.  In our dialect, we don't want to see an 
exception to throw exceptions. This is a rule for our dialect. And generally we 
have 2 strategies for implementing such language rules :

- Detect it and emit errors after we found users violating the rules.
- Don't detect it and claims it is the fault of the users.

No matter what the strategy we choose, it is not orthogonal to 
`-fassume-nothrow-exception-dtor` for sure. It is highly related. Then, I think 
the first strategy is always a better choice. Generally we only choose 2 when 
we can't diagnose such cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108905

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

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that all exception objects' destructors are non-throwing

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.

LGTM. Thanks.




Comment at: clang/lib/Sema/SemaExprCXX.cpp:1110
+!FT->isNothrow())
+  Diag(ThrowLoc, diag::err_throw_object_throwing_dtor) << Ty << 1 << 1;
+  }

It looks like err_throw_object_throwing_dtor doesn't require any parameters?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108905

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

[PATCH] D128328: [C++20][Modules] Improve handing of Private Module Fragment diagnostics.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

From the discussion, it looks like the 'export' part is not necessary here and 
we don't need to care about linkage in this revision.

In D128328#3609827 , @vsapsai wrote:

> Sorry for changing my mind. I've thought about the errors more and especially 
> about the case mentioned by Chuanqi
>
>   export module A;
>   [export] inline void func();
>
> I'm afraid it can complicate the implementation but we can achieve some 
> consistency with errors like
>
>   export module A;
>   export inline void func(); // error: no definition for exported inline 
> function 'func' in module 'A'
>
> and
>
>   export module A;
>   export inline void func(); // error: no definition for exported inline 
> function 'func' in module 'A'
>   //...
>   module :private;
>   void func() {}  // note: definition here is not reachable as it is private
>
> I think it is useful to have connection between declaration and definition 
> and to explain why the definition is no good.
>
> Specific wording around "no definition | missing definition | definition 
> required" is flexible.

It makes sense to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128328

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

[PATCH] D128487: [ODRHash diagnostics] Move repetetive code at lambda calls into lambdas themselves. NFC.

[Chuanqi Xu via Phabricator via cfe-commits]

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

LGTM. I found the codes about ODR Check was repeated too. It should be good to 
do such refactoring.

---

Is it possible to combine the several `DiagNote` into `DiagError`? So that the 
code would be further reduced. I am OK to do this kind of change in other 
revisions.




Comment at: clang/lib/Serialization/ASTReader.cpp:9827
 return false;
-  };
+  };
 

Unitentional change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128487

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

[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 440847.
ChuanqiXu marked 15 inline comments as done.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D113545

Files:
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Basic/Module.h
  clang/include/clang/Sema/Lookup.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaCXXScopeSpec.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaModule.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  
clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p4-friend-in-reachable-class.cpp
  clang/test/CXX/basic/basic.scope/basic.scope.namespace/p2.cpp
  clang/test/CXX/module/module.context/p7.cpp
  clang/test/CXX/module/module.import/p2.cpp
  clang/test/CXX/module/module.interface/p2.cpp
  clang/test/CXX/module/module.interface/p7.cpp
  clang/test/CXX/module/module.reach/ex1.cpp
  clang/test/CXX/module/module.reach/p2.cpp
  clang/test/CXX/module/module.reach/p4/TransitiveImport.cpp
  clang/test/CXX/module/module.reach/p5.cpp
  clang/test/CXX/module/module.unit/p7/t6.cpp
  clang/test/CXX/modules-ts/basic/basic.link/p2/other.cpp
  clang/test/Modules/Reachability-Private.cpp
  clang/test/Modules/Reachability-func-default-arg.cpp
  clang/test/Modules/Reachability-func-ret.cpp
  clang/test/Modules/Reachability-template-default-arg.cpp
  clang/test/Modules/Reachability-template-instantiation.cpp
  clang/test/Modules/Reachability-using-templates.cpp
  clang/test/Modules/Reachability-using.cpp
  clang/test/Modules/cxx20-10-1-ex2.cpp
  clang/test/Modules/derived_class.cpp
  clang/test/Modules/explicitly-specialized-template.cpp
  clang/test/Modules/module-private.cpp
  clang/test/Modules/template-function-specialization.cpp
  clang/test/Modules/template_default_argument.cpp

Index: clang/test/Modules/template_default_argument.cpp
===
--- /dev/null
+++ clang/test/Modules/template_default_argument.cpp
@@ -0,0 +1,29 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/B.cppm -emit-module-interface -o %t/B.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cpp -fsyntax-only -verify
+//
+//--- templ.h
+template 
+class templ {};
+template 
+void templ_func() {}
+
+//--- B.cppm
+module;
+#include "templ.h"
+export module B;
+export template 
+templ bar() {
+  templ_func();
+  return {};
+}
+
+//--- Use.cpp
+// expected-no-diagnostics
+import B;
+auto foo() {
+  return bar();
+}
Index: clang/test/Modules/template-function-specialization.cpp
===
--- /dev/null
+++ clang/test/Modules/template-function-specialization.cpp
@@ -0,0 +1,60 @@
+// RUN: rm -fr %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/foo.cppm -o %t/foo.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cpp -verify -fsyntax-only
+//
+//--- foo.cppm
+module;
+# 3 __FILE__ 1 // use the next physical line number here (and below)
+template 
+void foo() {
+}
+
+template <>
+void foo() {
+}
+
+template 
+void foo2() {
+}
+
+template <>
+void foo2() {
+}
+
+template 
+void foo3() {
+}
+
+template <>
+void foo3();
+
+export module foo;
+export using ::foo;
+export using ::foo3;
+
+export template 
+void foo4() {
+}
+
+export template <>
+void foo4() {
+}
+
+//--- Use.cpp
+import foo;
+void use() {
+  foo();
+  foo();
+  foo2(); // expected-error {{missing '#include'; 'foo2' must be declared before it is used}}
+ // expected-note@* {{declaration here is not visible}}
+  foo2();   // expected-error {{missing '#include'; 'foo2' must be declared before it is used}}
+ // expected-note@* {{declaration here is not visible}}
+  foo3();
+  foo3();
+
+  foo4();
+  foo4();
+}
Index: clang/test/Modules/module-private.cpp
===
--- clang/test/Modules/module-private.cpp
+++ clang/test/Modules/module-private.cpp
@@ -21,8 +21,8 @@
   vector vec; // expected-error{{no template named 'vector'}}
 
   VisibleStruct vs;
-  vs.field = 0; // expected-error{{no member named 'field' in 'VisibleStruct'}}
-  vs.setField(1); // expected-error{{no member named 'setField' in 'VisibleStruct'}}
+  vs.field = 0;
+  vs.setField(1);
 
   return hidden_var; // expected-error{{use of undeclared identifier 'hidden_var'}}
 }
Index: clang/test/Modules/explicitly-specialized-template.cpp
===
--- /dev/null
+++ clang/test/Modules/explicitly-specialized-template.cpp
@@ -0,0 +1,47 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: 

[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D113545#3616814 , @rsmith wrote:

> A few comments, but they're all minor things or FIXMEs. I'm happy for this to 
> land once they're addressed.

Thanks for reviewing!




Comment at: clang/lib/Sema/SemaLookup.cpp:2000-2004
+  // Class and enumeration member names can be found by name lookup in any
+  // context in which a definition of the type is reachable.
+  if (auto *ECD = dyn_cast(ND))
+return getSema().hasReachableDeclaration(
+cast(ECD->getDeclContext()));

rsmith wrote:
> ChuanqiXu wrote:
> > rsmith wrote:
> > > I don't think this is quite right. Given:
> > > 
> > > ```
> > > export module M {
> > > export enum E1 { e1 };
> > > enum E2 { e2 };
> > > export using E2U = E2;
> > > enum E3 { e3 };
> > > export E3 f();
> > > ```
> > > 
> > > ... the intent is:
> > > 
> > > ```
> > > import M;
> > > int main() {
> > >   auto a = E1::e1; // OK, namespace-scope name E1 is visible and e1 is 
> > > reachable
> > >   auto b = e1; // OK, namespace-scope name e1 is visible
> > >   auto c = E2::e2; // error, namespace-scope name E2 is not visible
> > >   auto d = e2; // error, namespace-scope name e2 is not visible
> > >   auto e = E2U::e2; // OK, namespace-scope name E2U is visible and E2::e2 
> > > is reachable
> > >   auto f = E3::e3; // error, namespace-scope name E3 is not visible
> > >   auto g = e3; // error, namespace-scope name e3 is not visible
> > >   auto h = decltype(f())::e3; // OK, namespace-scope name f is visible 
> > > and E3::e3 is reachable
> > > }
> > > ```
> > > 
> > > Instead of doing the check in this function, I think we need to consider 
> > > the scope in which we're doing a lookup: if that scope is a class or 
> > > enumeration scope, and the class or enumeration has a reachable 
> > > definition, then we don't do any visibility checks for its members.
> > Oh, your example makes sense. The current implementation would accept `d` 
> > and `g` unexpectedly. I spent some time to look into this one. And I find 
> > it is not so easy to fix. I left a FIXME below and I would file an issue if 
> > this patch landed. Do you think this is OK?
> > 
> > BTW, I feel like the wording of spec need to be adjusted too. From the 
> > wording, I feel like `d` and `g` should be accepted.
> Yes, I'm fine with having FIXMEs for things that don't work yet -- so long as 
> we don't regress things (particularly, so long as we don't regress module map 
> module support, which some of our users are heavily relying on).
> 
> I agree that the wording here is not capturing the rules properly; I've 
> mailed the CWG reflector and it sounds like Davis is going to fix that :)
Thanks! I believe it wouldn't be a regression.


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

https://reviews.llvm.org/D113545

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

[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

[Chuanqi Xu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9c04851cf580: [C++20] [Module] Support reachable definition 
initially/partially (authored by ChuanqiXu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113545

Files:
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Basic/Module.h
  clang/include/clang/Sema/Lookup.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaCXXScopeSpec.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaModule.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  
clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p4-friend-in-reachable-class.cpp
  clang/test/CXX/basic/basic.scope/basic.scope.namespace/p2.cpp
  clang/test/CXX/module/module.context/p7.cpp
  clang/test/CXX/module/module.import/p2.cpp
  clang/test/CXX/module/module.interface/p2.cpp
  clang/test/CXX/module/module.interface/p7.cpp
  clang/test/CXX/module/module.reach/ex1.cpp
  clang/test/CXX/module/module.reach/p2.cpp
  clang/test/CXX/module/module.reach/p4/TransitiveImport.cpp
  clang/test/CXX/module/module.reach/p5.cpp
  clang/test/CXX/module/module.unit/p7/t6.cpp
  clang/test/CXX/modules-ts/basic/basic.link/p2/other.cpp
  clang/test/Modules/Reachability-Private.cpp
  clang/test/Modules/Reachability-func-default-arg.cpp
  clang/test/Modules/Reachability-func-ret.cpp
  clang/test/Modules/Reachability-template-default-arg.cpp
  clang/test/Modules/Reachability-template-instantiation.cpp
  clang/test/Modules/Reachability-using-templates.cpp
  clang/test/Modules/Reachability-using.cpp
  clang/test/Modules/cxx20-10-1-ex2.cpp
  clang/test/Modules/derived_class.cpp
  clang/test/Modules/explicitly-specialized-template.cpp
  clang/test/Modules/module-private.cpp
  clang/test/Modules/template-function-specialization.cpp
  clang/test/Modules/template_default_argument.cpp

Index: clang/test/Modules/template_default_argument.cpp
===
--- /dev/null
+++ clang/test/Modules/template_default_argument.cpp
@@ -0,0 +1,29 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/B.cppm -emit-module-interface -o %t/B.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cpp -fsyntax-only -verify
+//
+//--- templ.h
+template 
+class templ {};
+template 
+void templ_func() {}
+
+//--- B.cppm
+module;
+#include "templ.h"
+export module B;
+export template 
+templ bar() {
+  templ_func();
+  return {};
+}
+
+//--- Use.cpp
+// expected-no-diagnostics
+import B;
+auto foo() {
+  return bar();
+}
Index: clang/test/Modules/template-function-specialization.cpp
===
--- /dev/null
+++ clang/test/Modules/template-function-specialization.cpp
@@ -0,0 +1,60 @@
+// RUN: rm -fr %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/foo.cppm -o %t/foo.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cpp -verify -fsyntax-only
+//
+//--- foo.cppm
+module;
+# 3 __FILE__ 1 // use the next physical line number here (and below)
+template 
+void foo() {
+}
+
+template <>
+void foo() {
+}
+
+template 
+void foo2() {
+}
+
+template <>
+void foo2() {
+}
+
+template 
+void foo3() {
+}
+
+template <>
+void foo3();
+
+export module foo;
+export using ::foo;
+export using ::foo3;
+
+export template 
+void foo4() {
+}
+
+export template <>
+void foo4() {
+}
+
+//--- Use.cpp
+import foo;
+void use() {
+  foo();
+  foo();
+  foo2(); // expected-error {{missing '#include'; 'foo2' must be declared before it is used}}
+ // expected-note@* {{declaration here is not visible}}
+  foo2();   // expected-error {{missing '#include'; 'foo2' must be declared before it is used}}
+ // expected-note@* {{declaration here is not visible}}
+  foo3();
+  foo3();
+
+  foo4();
+  foo4();
+}
Index: clang/test/Modules/module-private.cpp
===
--- clang/test/Modules/module-private.cpp
+++ clang/test/Modules/module-private.cpp
@@ -21,8 +21,8 @@
   vector vec; // expected-error{{no template named 'vector'}}
 
   VisibleStruct vs;
-  vs.field = 0; // expected-error{{no member named 'field' in 'VisibleStruct'}}
-  vs.setField(1); // expected-error{{no member named 'setField' in 'VisibleStruct'}}
+  vs.field = 0;
+  vs.setField(1);
 
   return hidden_var; // expected-error{{use of undeclared identifier 'hidden_var'}}
 }
Index: clang/test/Modules/explicitly-specialized-template.cpp
===
--- /dev/null
+

[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

@jyknight ping!

Or someone else is willing to review this one?


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

https://reviews.llvm.org/D125291

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

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/include/clang/AST/DeclBase.h:624
   bool isModulePrivate() const {
 return getModuleOwnershipKind() == ModuleOwnershipKind::ModulePrivate;
   }

According to the opinion from @rsmith, the discarded declaration is private too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694

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

[PATCH] D118034: [C++20] [Modules] Don't complain about duplicated default template argument across modules

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

@rsmith @iains gentle ping~


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

https://reviews.llvm.org/D118034

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126907#3615807 , @erichkeane 
wrote:

> All tests pass now, I was able to get the template-template checks working 
> correctly, and it passes all the tests I have available.  @ChuanqiXu if you 
> could review/run what tests you have, it would be greatly appreciated.

I've tested some of our workloads and libunifex (I know it contains a lot of 
uses for concept). And all the tests passed now. So it looks like it wouldn't 
cause regression failure.

The implementation **basically** looks good to me. (This is really large and I 
can't be sure I don't miss something). Only some minor issues remained.




Comment at: clang/include/clang/AST/Decl.h:1944-1948
+  /// For non-templates this value will be NULL, unless this non-template
+  /// function declaration was declared directly inside of a function template,
+  /// in which case this will have a pointer to a FunctionDecl, stored in the
+  /// NamedDecl. For function declarations that describe a function template,
+  /// this will be a pointer to a FunctionTemplateDecl, stored in the 
NamedDecl.

Could we give a new abstract like `XXXInfo` to replace `NamedDecl*` here? The 
NamedDecl* covers a lot of things. It looks more consistent.



Comment at: clang/include/clang/Sema/Sema.h:7056-7067
+  /*
+  // Keep track of whether we are evaluating a constraint.
+  unsigned ConstraintEvaluationDepth = 0;
+
+  class ConstraintEvalRAII {
+Sema &S;
+

Remove this before landing.



Comment at: clang/include/clang/Sema/Sema.h:7070
 public:
+  // bool IsEvaluatingAConstraint() { return ConstraintEvaluationDepth > 0; }
   const NormalizedConstraint *

ditto



Comment at: clang/lib/AST/Decl.cpp:3788
  "Member function is already a specialization");
-  TemplateOrSpecialization = Template;
+  TemplateOrSpecialization = static_cast(Template);
+}

`static_cast` is rarely used in clang/LLVM to me. I know the reason is that 
`TemplateOrSpecialization` contains a `NamedDecl*` member. I guess it might be 
better to refactor it.



Comment at: clang/lib/AST/DeclBase.cpp:286-304
 const DeclContext *Decl::getParentFunctionOrMethod() const {
   for (const DeclContext *DC = getDeclContext();
DC && !DC->isTranslationUnit() && !DC->isNamespace();
DC = DC->getParent())
 if (DC->isFunctionOrMethod())
   return DC;
 

How about:



Comment at: clang/lib/Sema/SemaConcept.cpp:63
+
+  ExprResult recreateBinOp(Sema &SemaRef, ExprResult LHS) {
+return recreateBinOp(SemaRef, LHS, const_cast(getRHS()));





Comment at: clang/lib/Sema/SemaConcept.cpp:67
+
+  ExprResult recreateBinOp(Sema &SemaRef, ExprResult LHS, ExprResult RHS) {
+assert((isAnd() || isOr()) && "Not the right kind of op?");





Comment at: clang/lib/Sema/SemaConcept.cpp:186
+return BO.recreateBinOp(S, LHSRes, RHSRes);
   } else if (auto *C = dyn_cast(ConstraintExpr)) {
+// These aren't evaluated, so we don't care about cleanups, so we can just





Comment at: clang/lib/Sema/SemaTemplate.cpp:2339
   if (const auto *TC = TTP->getTypeConstraint())
-SemaRef.SubstTypeConstraint(NewTTP, TC, Args);
+SemaRef.SubstTypeConstraint(NewTTP, TC, Args, false);
   if (TTP->hasDefaultArgument()) {




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

https://reviews.llvm.org/D126907

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

[PATCH] D127187: [C++20] [Modules] Implement AllAdditionalTUReachable

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 440872.
ChuanqiXu added a reviewer: MaskRay.
ChuanqiXu added a comment.
Herald added a subscriber: StephenFan.

Rebasing.


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

https://reviews.llvm.org/D127187

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/test/CXX/module/module.reach/ex1.cpp
  clang/test/CXX/module/module.reach/p2.cpp

Index: clang/test/CXX/module/module.reach/p2.cpp
===
--- clang/test/CXX/module/module.reach/p2.cpp
+++ clang/test/CXX/module/module.reach/p2.cpp
@@ -3,6 +3,7 @@
 // RUN: split-file %s %t
 // RUN: %clang_cc1 -std=c++20 %t/impl.cppm -emit-module-interface -o %t/M-impl.pcm
 // RUN: %clang_cc1 -std=c++20 %t/M.cppm -emit-module-interface -fprebuilt-module-path=%t -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/Use.cpp -fprebuilt-module-path=%t -verify -fsyntax-only -fall-additional-TU-reachable
 // RUN: %clang_cc1 -std=c++20 %t/UseStrict.cpp -fprebuilt-module-path=%t -verify -fsyntax-only
 
 //--- impl.cppm
@@ -14,6 +15,13 @@
 import :impl;
 export A f();
 
+//--- Use.cpp
+// expected-no-diagnostics
+import M;
+void test() {
+  auto a = f();
+}
+
 //--- UseStrict.cpp
 import M;
 void test() {
Index: clang/test/CXX/module/module.reach/ex1.cpp
===
--- clang/test/CXX/module/module.reach/ex1.cpp
+++ clang/test/CXX/module/module.reach/ex1.cpp
@@ -10,6 +10,7 @@
 //
 // RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/M.cppm -o %t/M.pcm
 // RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/X.cppm -fsyntax-only -verify
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/X-relax.cppm -fsyntax-only -verify -fall-additional-TU-reachable
 //
 //--- M-A.cppm
 export module M:A;
@@ -41,3 +42,11 @@
   // expected-note@* {{definition here is not reachable}} expected-note@* {{}}
 // FIXME: We should emit an error for unreachable definition of B.
 void g() { f(); }
+
+//--- X-relax.cppm
+// Tests that it wouldn't complain if we treat all additional TU as reachable TU.
+// expected-no-diagnostics
+export module X;
+import M;
+B b3;
+void g() { f(); }
Index: clang/lib/Sema/SemaLookup.cpp
===
--- clang/lib/Sema/SemaLookup.cpp
+++ clang/lib/Sema/SemaLookup.cpp
@@ -1953,9 +1953,9 @@
   //   considered reachable, but it is unspecified which are and under what
   //   circumstances.
   //
-  // The decision here is to treat all additional tranditional units as
-  // unreachable.
-  return false;
+  // The default value for AllAdditionalTUReachable is false. It implies all
+  // additional translation unit is not considered as reachable for portability.
+  return SemaRef.getLangOpts().AllAdditionalTUReachable;
 }
 
 bool Sema::isAcceptableSlow(const NamedDecl *D, Sema::AcceptableKind Kind) {
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3640,6 +3640,12 @@
options::OPT_fno_implicit_module_maps, HaveClangModules))
 CmdArgs.push_back("-fimplicit-module-maps");
 
+  // -fall-additional-TU-reachable enables to treat all translation unit as
+  // reachable.
+  if (Args.hasFlag(options::OPT_fall_additional_TU_reachable,
+   options::OPT_fno_all_additional_TU_reachable, false))
+CmdArgs.push_back("-fall-additional-TU-reachable");
+
   // -fmodules-decluse checks that modules used are declared so (off by default)
   Args.addOptInFlag(CmdArgs, options::OPT_fmodules_decluse,
 options::OPT_fno_modules_decluse);
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1428,6 +1428,10 @@
   NegFlag, PosFlag,
   BothFlags<[NoXarchOption], " modules for C++">>,
   ShouldParseIf;
+defm all_additional_TU_reachable : BoolFOption<"all-additional-TU-reachable",
+  LangOpts<"AllAdditionalTUReachable">, DefaultFalse,
+  NegFlag, PosFlag,
+  BothFlags<[NoXarchOption], " treat all additional variable as reachable">>;
 def fdebug_pass_arguments : Flag<["-"], "fdebug-pass-arguments">, Group;
 def fdebug_pass_structure : Flag<["-"], "fdebug-pass-structure">, Group;
 def fdepfile_entry : Joined<["-"], "fdepfile-entry=">,
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -171,6 +171,7 @@
 LANGOPT(Modules   , 1, 0, "modules semantics")
 COMPATIBLE_LANGOPT(ModulesTS  , 1, 0, "C++ Modules TS syntax")
 COM

[PATCH] D128487: [ODRHash diagnostics] Move repetetive code at lambda calls into lambdas themselves. NFC.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D128487#3617422 , @vsapsai wrote:

> Thanks for the review!
>
> In D128487#3614251 , @ChuanqiXu 
> wrote:
>
>> Is it possible to combine the several `DiagNote` into `DiagError`? So that 
>> the code would be further reduced. I am OK to do this kind of change in 
>> other revisions.
>
> Do you have any immediate ideas? I have more changes in this area (see the 
> stack), so I'm interested in improving this code. I've started thinking about 
> some approach that has less repetition but my initial approach was using 
> macros and it started to look pretty complicated without finishing the whole 
> change. So I've decided that the repetitive but simple code is easier to work 
> with than something complicated. But maybe you have some good ideas.

No immediate or concert ideas here.. It is hard to do refactoring. I sent 
https://reviews.llvm.org/D118437 before to do some simplification for the 
dispatch of default template argument. But I don't find a general method/idea 
to solve it in batch...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128487

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

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

@iains may I ask what's the issue to not land this? It looks like you're 
waiting for the behavior to be consistency with GCC?

Since this patch could fix https://github.com/llvm/llvm-project/issues/51873, 
which breaks the users to compile a hello world example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126189

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[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 D126907#3619270 , @erichkeane 
wrote:

> All but the 1 comment from @ChuanqiXu fixed, not sure what to do about the 
> 'info'.

LGTM. But the 'info' comment doesn't matter a lot. The current complexity is 
acceptable and its semantics is stated clearly in the comments. My intention is 
to make its semantics more clear and more readable so that it is easier to 
maintain and change it. But this is required. Since the patch is really large. 
So my preference is to do other unnecessary changes in other revision to make 
the patch easier to read. (Just a preference. I know some people prefer large 
patches.)




Comment at: clang/include/clang/AST/Decl.h:1944-1948
+  /// For non-templates this value will be NULL, unless this non-template
+  /// function declaration was declared directly inside of a function template,
+  /// in which case this will have a pointer to a FunctionDecl, stored in the
+  /// NamedDecl. For function declarations that describe a function template,
+  /// this will be a pointer to a FunctionTemplateDecl, stored in the 
NamedDecl.

erichkeane wrote:
> ChuanqiXu wrote:
> > Could we give a new abstract like `XXXInfo` to replace `NamedDecl*` here? 
> > The NamedDecl* covers a lot of things. It looks more consistent.
> All of those 'Info' types contain significantly more information, which we 
> really don't have a need for here.  Basically all this patch is doing is 
> using the FunctionTemplateDecl* that was already in the list and generalizing 
> it a bit.  AND unfortunately, the only commonality between 
> FunctionTemplateDecl and FunctionDecl is NamedDecl.  
> 
> So any type we would use would end up causing an additional allocation here, 
> and make its use require us to unpack it :/
> 
> I guess what I'm saying, is I'm not sure what that would look like in a way 
> that wouldn't make this worse.  Do you have an idea you could share that 
> would improve that?  
> 
> 
My idea would be something like:

```C++
llvm::PointerUnion So any type we would use would end up causing an additional allocation here, 
> and make its use require us to unpack it :/

This is a union so it wouldn't cause additional allocations?



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

https://reviews.llvm.org/D126907

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

[PATCH] D128488: [ODRHash diagnostics] Split `err_module_odr_violation_mismatch_decl_diff` into per-entity diagnostics. NFC.

[Chuanqi Xu via Phabricator via cfe-commits]

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

LGTM if the changes is specific to C++. Otherwise we need to rename for that 
enumerate.




Comment at: clang/lib/Serialization/ASTReader.cpp:9642
+  // note_module_odr_violation_record
+  enum ODRCXXRecordDifference {
 StaticAssertCondition,

Is this specific to C++?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128488

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

[PATCH] D128489: [ODRHash diagnostics] Move common code for calculating diag locations in `DiagnoseODRMismatch` into a lambda. NFC.

[Chuanqi Xu via Phabricator via cfe-commits]

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

LGTM with comments.




Comment at: clang/lib/Serialization/ASTReader.cpp:10020
+auto GetMismatchedDeclLoc = [](const NamedDecl *Container,
+   ODRMismatchDecl DiffType, const Decl *D) {
+  SourceLocation Loc;

DiffType is not necessary here, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128489

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

[PATCH] D128489: [ODRHash diagnostics] Move common code for calculating diag locations in `DiagnoseODRMismatch` into a lambda. NFC.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:10020
+auto GetMismatchedDeclLoc = [](const NamedDecl *Container,
+   ODRMismatchDecl DiffType, const Decl *D) {
+  SourceLocation Loc;

vsapsai wrote:
> ChuanqiXu wrote:
> > DiffType is not necessary here, right?
> Unfortunately, it is necessary. A little bit later we have
> ```
> if (DiffType == EndOfClass && Tag)
> ```
> where we use `DiffType`.
> 
> By the way, the implementation isn't really correct for `EndOfClass` and 
> non-TagDecl as `Loc` remains invalid. But that's a separate issue, I've 
> decided to keep the current change to NFC.
Oh, sorry for I missed that. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128489

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

[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a reviewer: ChuanqiXu.
ChuanqiXu added a comment.

Thanks for filing that issue. I wanted to add that myself before. It should be 
better to change `isVisible` and `hasVisibleDefinition` to `isReachable` and 
`hasReachableDefinition`. The definition of reachability comes from C++20 
Modules and it shouldn't affect Clang Modules.

And would you like to add an example for C++20 Modules? It should be something 
like:

  // RUN: rm -rf %t
  // RUN: mkdir %t
  // RUN: split-file %s %t
  //
  // RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/same_as.cppm -o 
%t/same_as.pcm
  // RUN: %clang_cc1 -std=c++20 -emit-module-interface 
-fprebuilt-module-path=%t %t/concepts.cppm -o %t/concepts.pcm
  // RUN: %clang_cc1 -std=c++20 -emit-module-interface 
-fprebuilt-module-path=%t %t/format.cppm -o %t/format.pcm
  // RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/conflicting.cppm -o 
%t/conflicting.pcm
  // RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cppm 
-fsyntax-only -verify
  
  //--- same_as.cppm
  export module same_as;
  export template 
  concept same_as = __is_same(T, U);
  
  //--- concepts.cppm
  export module concepts;
  export import same_as;
  
  export template 
  concept ConflictingConcept = true;
  
  //--- format.cppm
  
  export module format;
  export import concepts;
  export import same_as;
  
  export template  void foo()
requires same_as
  {}
  
  //--- conflicting.cppm
  export module conflicting;
  export template 
  concept ConflictingConcept = true;
  
  //--- Use.cppm
  import format;
  import conflicting;
  
  template  void foo()
requires same_as
  {}
  ConflictingConcept auto x = 10; // expected-error {{reference to 
'ConflictingConcept' is ambiguous}}
  // expected-note@* 2 {{candidate}}

Also I feel it would be better to rewrite the current tests into the above 
style by `split-file`.




Comment at: clang/lib/Sema/SemaTemplate.cpp:8679
+  Previous.isSingleResult() ? Previous.getAsSingle() : 
nullptr;
+  if (PrevDef && !hasVisibleDefinition(PrevDef) &&
+  Context.isSameEntity(NewDecl, PrevDef)) {





Comment at: clang/lib/Sema/SemaTemplate.cpp:8689
+  while (F.hasNext()) {
+if (!isVisible(F.next())) {
+  F.erase();




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128921

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

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126189#3617896 , @iains wrote:

> In D126189#3617850 , @ChuanqiXu 
> wrote:
>
>> @iains may I ask what's the issue to not land this? It looks like you're 
>> waiting for the behavior to be consistency with GCC?
>>
>> Since this patch could fix 
>> https://github.com/llvm/llvm-project/issues/51873, which breaks the users to 
>> compile a hello world example.
>
> I need to make some typo corrections; there is no issue (I'm not waiting for 
> anything) just prioritising posting patches to complete C++20 support .. will 
> land this soon.

Thanks! The reason why I am a little bit hurry is that I'm trying to implement 
std modules: https://github.com/ChuanqiXu9/stdmodules

Now it runs good for libcxx. But it would meet segfault in 
https://github.com/ChuanqiXu9/stdmodules/blob/master/test/HelloWorld/HelloWorld.cpp
 if we don't land this revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126189

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

[PATCH] D118034: [C++20] [Modules] Don't complain about duplicated default template argument across modules

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a reviewer: ilya-biryukov.
ChuanqiXu added a subscriber: ilya-biryukov.
ChuanqiXu added a comment.

Add @ilya-biryukov since he did similar things in 
https://reviews.llvm.org/D128921


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

https://reviews.llvm.org/D118034

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

[PATCH] D128974: [AST] [Modules] Handle full cases of DefaultArgStorage::setInherited

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu created this revision.
ChuanqiXu added reviewers: erichkeane, iains, vsapsai, v.g.vassilev, dblaikie, 
rsmith, jansvoboda11, aaron.ballman.
ChuanqiXu added projects: clang, clang-modules.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added a subscriber: cfe-commits.

There were two assertions in DefaultArgStorage::setInherited previously. But 
there are edge cases could hit them actually. One is 
`InheritDefaultArguments.cppm` that I found recently (there is another issue in 
the driver: https://github.com/llvm/llvm-project/issues/56327). Another one is 
pr31469.cpp, which was created fives years ago.

This patch tries to fix the two failures by handling more cases in 
DefaultArgStorage::setInherited. And I added assertions for sameness to keep 
correctness.

This is guaranteed to not introduce any breaking change since it lives in the 
path we wouldn't touch before.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128974

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/DeclTemplate.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Modules/InheritDefaultArguments.cppm
  clang/test/Modules/Inputs/PR31469/textual.h

Index: clang/test/Modules/Inputs/PR31469/textual.h
===
--- clang/test/Modules/Inputs/PR31469/textual.h
+++ clang/test/Modules/Inputs/PR31469/textual.h
@@ -4,7 +4,7 @@
   template  class allocator;
   template > class list;
   template  class __list_iterator {
-//template  friend class list; // causes another crash in ASTDeclReader::attachPreviousDecl
+template  friend class list;
 template  friend class list;
   };
   template  class __list_imp {};
Index: clang/test/Modules/InheritDefaultArguments.cppm
===
--- clang/test/Modules/InheritDefaultArguments.cppm
+++ clang/test/Modules/InheritDefaultArguments.cppm
@@ -2,8 +2,8 @@
 // RUN: split-file %s %t
 // RUN: cd %t
 //
-// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm
-// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -I%t %t/Use.cppm -verify -fsyntax-only
+// RUN: %clang -std=c++20 %t/A.cppm --precompile -o %t/A.pcm
+// RUN: %clang -std=c++20 -fprebuilt-module-path=%t -I%t %t/Use.cppm -Xclang -verify -S -o -
 
 //--- foo.h
 template 
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -2643,46 +2643,6 @@
   return false;
 }
 
-static bool hasSameDefaultTemplateArgument(NamedDecl *X, NamedDecl *Y) {
-  ASTContext &C = X->getASTContext();
-  // If the type parameter isn't the same already, we don't need to check the
-  // default argument further.
-  if (!C.isSameTemplateParameter(X, Y))
-return false;
-
-  if (auto *TTPX = dyn_cast(X)) {
-auto *TTPY = cast(Y);
-if (!TTPX->hasDefaultArgument() || !TTPY->hasDefaultArgument())
-  return false;
-
-return C.hasSameType(TTPX->getDefaultArgument(),
- TTPY->getDefaultArgument());
-  }
-
-  if (auto *NTTPX = dyn_cast(X)) {
-auto *NTTPY = cast(Y);
-if (!NTTPX->hasDefaultArgument() || !NTTPY->hasDefaultArgument())
-  return false;
-
-Expr *DefaultArgumentX = NTTPX->getDefaultArgument()->IgnoreImpCasts();
-Expr *DefaultArgumentY = NTTPY->getDefaultArgument()->IgnoreImpCasts();
-llvm::FoldingSetNodeID XID, YID;
-DefaultArgumentX->Profile(XID, C, /*Canonical=*/true);
-DefaultArgumentY->Profile(YID, C, /*Canonical=*/true);
-return XID == YID;
-  }
-
-  auto *TTPX = cast(X);
-  auto *TTPY = cast(Y);
-
-  if (!TTPX->hasDefaultArgument() || !TTPY->hasDefaultArgument())
-return false;
-
-  const TemplateArgument &TAX = TTPX->getDefaultArgument().getArgument();
-  const TemplateArgument &TAY = TTPY->getDefaultArgument().getArgument();
-  return C.hasSameTemplateName(TAX.getAsTemplate(), TAY.getAsTemplate());
-}
-
 /// Checks the validity of a template parameter list, possibly
 /// considering the template parameter list from a previous
 /// declaration.
@@ -2780,7 +2740,8 @@
 if (!OldTypeParm->getOwningModule() ||
 isModuleUnitOfCurrentTU(OldTypeParm->getOwningModule()))
   RedundantDefaultArg = true;
-else if (!hasSameDefaultTemplateArgument(OldTypeParm, NewTypeParm)) {
+else if (!getASTContext().isSameDefaultTemplateArgument(OldTypeParm,
+NewTypeParm)) {
   InconsistentDefaultArg = true;
   PrevModuleName =
   OldTypeParm->getImportedOwningModule()->getFullModuleName();
@@ -2832,8 +2793,8 @@
 if (!OldNonTypeParm->getOwningModule() ||
 isModuleUnitOfCurrentTU(OldNonTypeParm->getOwningModule()))
   RedundantDefaultArg = true;
-else if (!hasSameDefaultTemp

[PATCH] D129045: [C++20][Modules] Update handling of implicit inlines [P1779R3]

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/AST/TextNodeDumper.cpp:1723-1725
+  if (!D->isInlineSpecified() && D->isInlined()) {
+OS << " implicit-inline";
+  }

Is this necessary to this revision?



Comment at: clang/lib/Sema/SemaDecl.cpp:9362
+  //   to the global module.
+  if (getLangOpts().CPlusPlus20 || getLangOpts().CPlusPlus2b) {
+Module *M = NewFD->getOwningModule();

If I remember correctly, when we run `-std=c++2b`, `getLangOpts().CPlusPlus20` 
would be true too. So we could check `getLangOpts().CPlusPlus20` only here.



Comment at: clang/lib/Sema/SemaDecl.cpp:9363-9365
+Module *M = NewFD->getOwningModule();
+if (!M || M->isGlobalModule())
+  NewFD->setImplicitlyInline();

nit: this is not  required.



Comment at: clang/lib/Sema/SemaDecl.cpp:9657
+  //   in its class definition, it is inline.
+  if (getLangOpts().CPlusPlus20 || getLangOpts().CPlusPlus2b) {
+Module *M = NewFD->getOwningModule();

ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129045

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

[PATCH] D128981: [C++20][Modules] Implement include translation.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a subscriber: rsmith.
ChuanqiXu added inline comments.



Comment at: clang/include/clang/Lex/Preprocessor.h:434
+/// Saw a 'module' identifier.
+void handleModule(bool ATLTS) {
+  // This was the first module identifier and not preceded by any token

What's the meaning of `ATLTS`?



Comment at: clang/lib/Lex/PPDirectives.cpp:2226-2227
+
+  // FIXME: We do not have a good way to disambiguate C++ clang modules from
+  // C++ standard modules (other than use/non-use of Header Units).
+  Module *SM = SuggestedModule.getModule();

From what @rsmith said in https://reviews.llvm.org/D113391, it looks like the 
ideal direction is to use C++ clang modules and C++ standard modules together. 
So it looks like we couldn't disambiguate them from command line options.



Comment at: clang/test/Modules/cxx20-include-translation.cpp:10
+// RUN: %clang_cc1 -std=c++20 -xc++-user-header h4.h -emit-header-unit -o 
h4.pcm
+
+// RUN: %clang_cc1 -std=c++20 Xlate.cpp -emit-module-interface -o Xlate.pcm \

Maybe we need an example `h5.h` which is not pre-compiled  as a header unit and 
it would be handled correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128981

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

[PATCH] D129045: [C++20][Modules] Update handling of implicit inlines [P1779R3]

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

It looks like the tests lack the cast that the methods are attached to the 
global module fragment.




Comment at: clang/lib/Sema/SemaDecl.cpp:9363-9365
+Module *M = NewFD->getOwningModule();
+if (!M || M->isGlobalModule())
+  NewFD->setImplicitlyInline();

iains wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > nit: this is not  required.
> > well, the logic is required ;) .. you have suggested a different way to 
> > write..
> actually, I think you meant `!NewFD->getOwningModule() || 
> NewFD->getOwningModule()->isGlobalModule()` but I've pulled the test out so 
> that it's only done once.
> 
Yeah, thanks!



Comment at: clang/test/AST/ast-dump-constant-expr.cpp:94
+// CHECK-NEXT:`-CXXMethodDecl {{.*}}  col:18 consteval 
consteval_method 'void ()' implicit-inline
\ No newline at end of file


Maybe it is good to add the newline although it is not your fault.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129045

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

[PATCH] D128981: [C++20][Modules] Implement include translation.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/include/clang/Lex/Preprocessor.h:434
+/// Saw a 'module' identifier.
+void handleModule(bool ATLTS) {
+  // This was the first module identifier and not preceded by any token

iains wrote:
> ChuanqiXu wrote:
> > What's the meaning of `ATLTS`?
> `AfterTopLevelTokenSeq` I guess it seemed a rather long name (but I do not 
> mind to spell it out if that makes sense).
> 
Oh, I feel better to spell it out.



Comment at: clang/lib/Lex/PPDirectives.cpp:2226-2227
+
+  // FIXME: We do not have a good way to disambiguate C++ clang modules from
+  // C++ standard modules (other than use/non-use of Header Units).
+  Module *SM = SuggestedModule.getModule();

iains wrote:
> ChuanqiXu wrote:
> > From what @rsmith said in https://reviews.llvm.org/D113391, it looks like 
> > the ideal direction is to use C++ clang modules and C++ standard modules 
> > together. So it looks like we couldn't disambiguate them from command line 
> > options.
> Well, I think there is some more debate to have around how to solve this 
> problem (i.e. it might be intended that clang++ modules and standard c++ 
> modules converge but as things stand we still need to support the cases that 
> they have different behaviour, or break existing users) 
>  ... - but please let us not have that debate in this patch :-)
> 
It is not good to break existing users. Generally, a breaking change patch 
could be reverted directly... We must care about it to avoid unnecessary 
efforts. And it looks like the current implementation wouldn't break any 
existing users, right? Since it uses `isHeaderUnit()`. I remember `HeaderUnit` 
is introduced by  you so it shouldn't conflict with clang modules.

BTW, may I ask the behavior is consistency with GCC?



Comment at: clang/lib/Lex/PPDirectives.cpp:2335
+ IsFirstIncludeOfFile)) {
+// standard modules:
+// If we are not in the GMF, then we textually include only

nit: It looks like we prefer to use `C++20 modules` over `standard modules`, 
although `standard modules` must be the right term.



Comment at: clang/lib/Parse/Parser.cpp:672
+if (!getLangOpts().CPlusPlusModules || !Mod->isHeaderUnit() ||
+getLangOpts().ModulesTS)
+  Actions.ActOnModuleInclude(Loc, Mod);

I think we could ignore `getLangOpts().ModulesTS` here.



Comment at: clang/test/Modules/cxx20-include-translation.cpp:10
+// RUN: %clang_cc1 -std=c++20 -xc++-user-header h4.h -emit-header-unit -o 
h4.pcm
+
+// RUN: %clang_cc1 -std=c++20 Xlate.cpp -emit-module-interface -o Xlate.pcm \

iains wrote:
> ChuanqiXu wrote:
> > Maybe we need an example `h5.h` which is not pre-compiled  as a header unit 
> > and it would be handled correctly.
> I am not sure if you mean that the un-converted header would be included in 
> the GMF or in the module purview - IMO we already have test-cases that cover 
> this, but I do not mind adding something extra if there's a missing case?
> 
> I am not sure if you mean that the un-converted header would be included in 
> the GMF or in the module purview 

At least in the GMF. 

> IMO we already have test-cases that cover this, but I do not mind adding 
> something extra if there's a missing case?

Yeah, I feel better to have more coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128981

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

[PATCH] D129045: [C++20][Modules] Update handling of implicit inlines [P1779R3]

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D129045#3627878 , @iains wrote:

> In D129045#3627856 , @ChuanqiXu 
> wrote:
>
>> It looks like the tests lack the cast that the methods are attached to the 
>> global module fragment.
>
> (maybe I misunderstand what you are saying here)
>
>   bool ImplicitInlineCXX20 = !getLangOpts().CPlusPlus20 ||
>  !NewFD->getOwningModule() ||
>  NewFD->getOwningModule()->isGlobalModule();
>
> We are in the global module if there is no owning module, or if the owning 
> module is the GMF. 
>  We always set this to true before C++20, matching the unconditional true 
> default argument it the setImplicitlyInline() method.

I mean the tests instead of the implementation. It looks like there is no GMF 
in the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129045

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

[PATCH] D129068: [AST] Profiling on constraint expression instead of arguments for TypeConstraint in ASTContext::isSameTemplateParameter

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu created this revision.
ChuanqiXu added reviewers: rsmith, vsapsai, ilya-biryukov.
ChuanqiXu added a project: clang-modules.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The current implementation to judge the similarity of TypeConstraint in 
ASTContext::isSameTemplateParameter is problematic, it couldn't handle the 
following case:

  C++
  template <__integer_like _Tp, C<_Tp> Sentinel>
  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
  return __t;
  }

When we see 2 such declarations from different modules, we would judge their 
similarity by `ASTContext::isSame*` methods. But problems come for the 
TypeConstraint. Originally, we would profile each argument one by one. But it 
is not right. Since the profiling result of `_Tp` would refer to two different 
template type declarations. So it would get different results. It is right 
since the `_Tp` in different modules refers to different declarations indeed. 
So the same declaration in different modules would meet incorrect our-checking 
results.

It is not the thing we want. We want to know if the TypeConstraint have the 
same expression.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129068

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/Modules/concept.cppm


Index: clang/test/Modules/concept.cppm
===
--- clang/test/Modules/concept.cppm
+++ clang/test/Modules/concept.cppm
@@ -18,6 +18,9 @@
 template 
 concept __member_size = requires(_Tp &&t) { t.size(); };
 
+template 
+concept C = requires(First x, Second y) { x+y; };
+
 struct A {
 public:
   template 
@@ -29,6 +32,11 @@
   constexpr __integer_like auto operator()(_Tp&& __t) const {
 return __t.size();
   }
+
+  template <__integer_like _Tp, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+return __t;
+  }
 };
 #endif
 
@@ -51,4 +59,5 @@
 auto operator+(S s) { return 0; }
 };
 __fn{}(S());
+__fn{}(S(), S());
 }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6245,11 +6245,14 @@
 auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
 if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
   return false;
+
 llvm::FoldingSetNodeID XID, YID;
-for (auto &ArgLoc : TXTCArgs->arguments())
-  ArgLoc.getArgument().Profile(XID, X->getASTContext());
-for (auto &ArgLoc : TYTCArgs->arguments())
-  ArgLoc.getArgument().Profile(YID, Y->getASTContext());
+auto *XConstraint = TXTC->getImmediatelyDeclaredConstraint();
+auto *YConstraint = TYTC->getImmediatelyDeclaredConstraint();
+if (XConstraint)
+  XConstraint->Profile(XID, *this, /*Canonical=*/true);
+if (YConstraint)
+  YConstraint->Profile(YID, *this, /*Canonical=*/true);
 if (XID != YID)
   return false;
   }
@@ -6524,10 +6527,10 @@
   // and patterns match.
   if (const auto *TemplateX = dyn_cast(X)) {
 const auto *TemplateY = cast(Y);
-return isSameEntity(TemplateX->getTemplatedDecl(),
-TemplateY->getTemplatedDecl()) &&
-   isSameTemplateParameterList(TemplateX->getTemplateParameters(),
-   TemplateY->getTemplateParameters());
+return  isSameEntity(TemplateX->getTemplatedDecl(),
+ TemplateY->getTemplatedDecl()) &&
+isSameTemplateParameterList(TemplateX->getTemplateParameters(),
+TemplateY->getTemplateParameters());
   }
 
   // Fields with the same name and the same type match.


Index: clang/test/Modules/concept.cppm
===
--- clang/test/Modules/concept.cppm
+++ clang/test/Modules/concept.cppm
@@ -18,6 +18,9 @@
 template 
 concept __member_size = requires(_Tp &&t) { t.size(); };
 
+template 
+concept C = requires(First x, Second y) { x+y; };
+
 struct A {
 public:
   template 
@@ -29,6 +32,11 @@
   constexpr __integer_like auto operator()(_Tp&& __t) const {
 return __t.size();
   }
+
+  template <__integer_like _Tp, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+return __t;
+  }
 };
 #endif
 
@@ -51,4 +59,5 @@
 auto operator+(S s) { return 0; }
 };
 __fn{}(S());
+__fn{}(S(), S());
 }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6245,11 +6245,14 @@
 auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
 if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
   return false;
+
 llvm::FoldingSetNodeI

[PATCH] D129068: [AST] Profiling on constraint expression instead of arguments for TypeConstraint in ASTContext::isSameTemplateParameter

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 442055.
ChuanqiXu added a comment.

Remove unnecessary changes.


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

https://reviews.llvm.org/D129068

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/Modules/concept.cppm


Index: clang/test/Modules/concept.cppm
===
--- clang/test/Modules/concept.cppm
+++ clang/test/Modules/concept.cppm
@@ -18,6 +18,9 @@
 template 
 concept __member_size = requires(_Tp &&t) { t.size(); };
 
+template 
+concept C = requires(First x, Second y) { x+y; };
+
 struct A {
 public:
   template 
@@ -29,6 +32,11 @@
   constexpr __integer_like auto operator()(_Tp&& __t) const {
 return __t.size();
   }
+
+  template <__integer_like _Tp, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+return __t;
+  }
 };
 #endif
 
@@ -51,4 +59,5 @@
 auto operator+(S s) { return 0; }
 };
 __fn{}(S());
+__fn{}(S(), S());
 }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6245,11 +6245,14 @@
 auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
 if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
   return false;
+
 llvm::FoldingSetNodeID XID, YID;
-for (auto &ArgLoc : TXTCArgs->arguments())
-  ArgLoc.getArgument().Profile(XID, X->getASTContext());
-for (auto &ArgLoc : TYTCArgs->arguments())
-  ArgLoc.getArgument().Profile(YID, Y->getASTContext());
+auto *XConstraint = TXTC->getImmediatelyDeclaredConstraint();
+auto *YConstraint = TYTC->getImmediatelyDeclaredConstraint();
+if (XConstraint)
+  XConstraint->Profile(XID, *this, /*Canonical=*/true);
+if (YConstraint)
+  YConstraint->Profile(YID, *this, /*Canonical=*/true);
 if (XID != YID)
   return false;
   }


Index: clang/test/Modules/concept.cppm
===
--- clang/test/Modules/concept.cppm
+++ clang/test/Modules/concept.cppm
@@ -18,6 +18,9 @@
 template 
 concept __member_size = requires(_Tp &&t) { t.size(); };
 
+template 
+concept C = requires(First x, Second y) { x+y; };
+
 struct A {
 public:
   template 
@@ -29,6 +32,11 @@
   constexpr __integer_like auto operator()(_Tp&& __t) const {
 return __t.size();
   }
+
+  template <__integer_like _Tp, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+return __t;
+  }
 };
 #endif
 
@@ -51,4 +59,5 @@
 auto operator+(S s) { return 0; }
 };
 __fn{}(S());
+__fn{}(S(), S());
 }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6245,11 +6245,14 @@
 auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
 if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
   return false;
+
 llvm::FoldingSetNodeID XID, YID;
-for (auto &ArgLoc : TXTCArgs->arguments())
-  ArgLoc.getArgument().Profile(XID, X->getASTContext());
-for (auto &ArgLoc : TYTCArgs->arguments())
-  ArgLoc.getArgument().Profile(YID, Y->getASTContext());
+auto *XConstraint = TXTC->getImmediatelyDeclaredConstraint();
+auto *YConstraint = TYTC->getImmediatelyDeclaredConstraint();
+if (XConstraint)
+  XConstraint->Profile(XID, *this, /*Canonical=*/true);
+if (YConstraint)
+  YConstraint->Profile(YID, *this, /*Canonical=*/true);
 if (XID != YID)
   return false;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128981: [C++20][Modules] Implement include translation.

[Chuanqi Xu via Phabricator via cfe-commits]

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

Then LGTM if all the comments addressed.




Comment at: clang/lib/Lex/PPDirectives.cpp:2226-2227
+
+  // FIXME: We do not have a good way to disambiguate C++ clang modules from
+  // C++ standard modules (other than use/non-use of Header Units).
+  Module *SM = SuggestedModule.getModule();

iains wrote:
> ChuanqiXu wrote:
> > iains wrote:
> > > ChuanqiXu wrote:
> > > > From what @rsmith said in https://reviews.llvm.org/D113391, it looks 
> > > > like the ideal direction is to use C++ clang modules and C++ standard 
> > > > modules together. So it looks like we couldn't disambiguate them from 
> > > > command line options.
> > > Well, I think there is some more debate to have around how to solve this 
> > > problem (i.e. it might be intended that clang++ modules and standard c++ 
> > > modules converge but as things stand we still need to support the cases 
> > > that they have different behaviour, or break existing users) 
> > >  ... - but please let us not have that debate in this patch :-)
> > > 
> > It is not good to break existing users. Generally, a breaking change patch 
> > could be reverted directly... We must care about it to avoid unnecessary 
> > efforts. And it looks like the current implementation wouldn't break any 
> > existing users, right? Since it uses `isHeaderUnit()`. I remember 
> > `HeaderUnit` is introduced by  you so it shouldn't conflict with clang 
> > modules.
> > 
> > BTW, may I ask the behavior is consistency with GCC?
> > It is not good to break existing users. Generally, a breaking change patch 
> > could be reverted directly... We must care about it to avoid unnecessary 
> > efforts. And it looks like the current implementation wouldn't break any 
> > existing users, right? Since it uses `isHeaderUnit()`. I remember 
> > `HeaderUnit` is introduced by  you so it shouldn't conflict with clang 
> > modules.
> 
> correct, in this case, the fact that Header Units are specific to the  C++20 
> implementation (they are quite different from clang header modules) allows us 
> to tell the difference.
> 
> > BTW, may I ask the behavior is consistency with GCC?
> 
> yes, I discussed this with @urnathan (especially that it is very difficult to 
> get consistent behaviour if we were to include-translate in the module 
> purview).
Got it. Thanks.



Comment at: clang/lib/Lex/PPDirectives.cpp:2335
+ IsFirstIncludeOfFile)) {
+// standard modules:
+// If we are not in the GMF, then we textually include only

iains wrote:
> ChuanqiXu wrote:
> > nit: It looks like we prefer to use `C++20 modules` over `standard 
> > modules`, although `standard modules` must be the right term.
> since we are heading for C++23  ... perhaps now would be a good time to start 
> using "standard modules"? (I can change to C++20 modules if there's 
> objection).
> 
It is OK to me. But I feel better if we use `C++ standard modules` since 
modules is a feature lives everywhere.



Comment at: clang/lib/Parse/Parser.cpp:672
+if (!getLangOpts().CPlusPlusModules || !Mod->isHeaderUnit() ||
+getLangOpts().ModulesTS)
+  Actions.ActOnModuleInclude(Loc, Mod);

iains wrote:
> ChuanqiXu wrote:
> > I think we could ignore `getLangOpts().ModulesTS` here.
> well, we agreed that (for the present) we would try to avoid changing the 
> behaviour w.r.t modules-ts (but spend no effort to complete the 
> implementation) - and to remove it from comments; we can then take a pass 
> through to remove the modules-ts behaviour (assuming that no-one is using it!)
> 
Given the previous status of C++ modules, it is hard to believe there is 
existing users for it. So I think it should be fine to remove the support for 
module-ts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128981

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

[PATCH] D129045: [C++20][Modules] Update handling of implicit inlines [P1779R3]

[Chuanqi Xu via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129045

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

[PATCH] D129068: [AST] Profiling on constraint expression instead of arguments for TypeConstraint in ASTContext::isSameTemplateParameter

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 442165.
ChuanqiXu added a comment.

Add comments.


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

https://reviews.llvm.org/D129068

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/Modules/concept.cppm


Index: clang/test/Modules/concept.cppm
===
--- clang/test/Modules/concept.cppm
+++ clang/test/Modules/concept.cppm
@@ -18,6 +18,9 @@
 template 
 concept __member_size = requires(_Tp &&t) { t.size(); };
 
+template 
+concept C = requires(First x, Second y) { x+y; };
+
 struct A {
 public:
   template 
@@ -29,6 +32,11 @@
   constexpr __integer_like auto operator()(_Tp&& __t) const {
 return __t.size();
   }
+
+  template <__integer_like _Tp, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+return __t;
+  }
 };
 #endif
 
@@ -51,4 +59,5 @@
 auto operator+(S s) { return 0; }
 };
 __fn{}(S());
+__fn{}(S(), S());
 }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6245,14 +6245,29 @@
 auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
 if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
   return false;
-llvm::FoldingSetNodeID XID, YID;
-for (auto &ArgLoc : TXTCArgs->arguments())
-  ArgLoc.getArgument().Profile(XID, X->getASTContext());
-for (auto &ArgLoc : TYTCArgs->arguments())
-  ArgLoc.getArgument().Profile(YID, Y->getASTContext());
-if (XID != YID)
-  return false;
+// We couldn't compare the profiling result for the template
+// args here. Consider the following example in different modules:
+//
+// template <__integer_like _Tp, C<_Tp> Sentinel>
+// constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+//   return __t;
+// }
+//
+// When we compare the profiling result for `C<_Tp>` in different
+// modules, it will compare the type of `_Tp` in different modules.
+// However, the type of `_Tp` in different modules refer to different
+// types here naturally. So we couldn't compare the profiling result
+// for the template args directly.
   }
+  llvm::FoldingSetNodeID XID, YID;
+  auto *XConstraint = TXTC->getImmediatelyDeclaredConstraint();
+  auto *YConstraint = TYTC->getImmediatelyDeclaredConstraint();
+  if (XConstraint)
+XConstraint->Profile(XID, *this, /*Canonical=*/true);
+  if (YConstraint)
+YConstraint->Profile(YID, *this, /*Canonical=*/true);
+  if (XID != YID)
+return false;
 }
 return true;
   }


Index: clang/test/Modules/concept.cppm
===
--- clang/test/Modules/concept.cppm
+++ clang/test/Modules/concept.cppm
@@ -18,6 +18,9 @@
 template 
 concept __member_size = requires(_Tp &&t) { t.size(); };
 
+template 
+concept C = requires(First x, Second y) { x+y; };
+
 struct A {
 public:
   template 
@@ -29,6 +32,11 @@
   constexpr __integer_like auto operator()(_Tp&& __t) const {
 return __t.size();
   }
+
+  template <__integer_like _Tp, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+return __t;
+  }
 };
 #endif
 
@@ -51,4 +59,5 @@
 auto operator+(S s) { return 0; }
 };
 __fn{}(S());
+__fn{}(S(), S());
 }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6245,14 +6245,29 @@
 auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
 if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
   return false;
-llvm::FoldingSetNodeID XID, YID;
-for (auto &ArgLoc : TXTCArgs->arguments())
-  ArgLoc.getArgument().Profile(XID, X->getASTContext());
-for (auto &ArgLoc : TYTCArgs->arguments())
-  ArgLoc.getArgument().Profile(YID, Y->getASTContext());
-if (XID != YID)
-  return false;
+// We couldn't compare the profiling result for the template
+// args here. Consider the following example in different modules:
+//
+// template <__integer_like _Tp, C<_Tp> Sentinel>
+// constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+//   return __t;
+// }
+//
+// When we compare the profiling result for `C<_Tp>` in different
+// modules, it will compare the type of `_Tp` in different modules.
+// However, the type of `_Tp` in different modules refer to different
+// types here naturally. So we couldn't compare the profiling result
+// for the template args directly.
   }
+  llvm::FoldingSetNodeID XID, YID;
+  auto *XCons

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

BTW, after I applied the patch, the compiler crashes at 
https://github.com/ChuanqiXu9/stdmodules. I would try to add more tests about 
C++20 Modules.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694

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

[PATCH] D129104: [Modules] Add ODR Check for concepts

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu created this revision.
ChuanqiXu added reviewers: ilya-biryukov, erichkeane, vsapsai, rsmith.
ChuanqiXu added a project: clang-modules.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Closing https://github.com/llvm/llvm-project/issues/56310

Previously we don't check the contents of concept so it might merge 
inconsistent results.

Important Note: this patch might break existing code but the behavior might be 
right.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129104

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/Modules/concept_differ.cpp
  clang/test/Modules/concept_differ.cppm

Index: clang/test/Modules/concept_differ.cppm
===
--- /dev/null
+++ clang/test/Modules/concept_differ.cppm
@@ -0,0 +1,43 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -x c++ -std=c++20 %t/A.cppm -I%t -emit-module-interface -o %t/A.pcm
+// RUN: %clang_cc1 -x c++ -std=c++20 %t/B.cppm -I%t -emit-module-interface -o %t/B.pcm
+// RUN: %clang_cc1 -x c++ -std=c++20 -fprebuilt-module-path=%t %t/foo.cpp -verify
+
+//--- foo.h
+template 
+concept A = true;
+
+//--- bar.h
+template 
+concept A = false;
+
+//--- A.cppm
+module;
+#include "foo.h"
+export module A;
+export using ::A;
+
+//--- B.cppm
+module;
+#include "bar.h"
+export module B;
+export using ::A;
+
+//--- foo.cpp
+import A;
+import B;
+
+template  void foo()
+  requires A
+{}   // expected-error 1+{{reference to 'A' is ambiguous}}
+ // expected-note@* 1+{{candidate found by name lookup}}
+template  void bar() {} // expected-error {{reference to 'A' is ambiguous}}
+
+int main() {
+  foo();
+  bar();
+  return 0;
+}
Index: clang/test/Modules/concept_differ.cpp
===
--- /dev/null
+++ clang/test/Modules/concept_differ.cpp
@@ -0,0 +1,37 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -x c++ -std=c++20 -fmodules -fmodules-cache-path=%t -fmodule-map-file=%t/module.map %t/foo.cpp -verify
+
+//--- module.map
+module "foo" {
+export *
+header "foo.h"} module "bar" {
+  export *
+  header "bar.h"
+}
+
+//--- foo.h
+template 
+concept A = true;
+
+//--- bar.h
+template 
+concept A = false;
+
+//--- foo.cpp
+#include "bar.h"
+#include "foo.h"
+
+template  void foo()
+  requires A
+{}   // expected-error 1+{{reference to 'A' is ambiguous}}
+ // expected-note@* 1+{{candidate found by name lookup}}
+template  void bar() {} // expected-error {{reference to 'A' is ambiguous}}
+
+int main() {
+  foo();
+  bar();
+  return 0;
+}
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6524,6 +6524,20 @@
   // and patterns match.
   if (const auto *TemplateX = dyn_cast(X)) {
 const auto *TemplateY = cast(Y);
+
+// ConceptDecl wouldn't be the same if their constraint expression differs.
+if (const auto *ConceptX = dyn_cast(X)) {
+  const auto *ConceptY = dyn_cast(Y);
+  const Expr *XCE = ConceptX->getConstraintExpr();
+  const Expr *YCE = ConceptY->getConstraintExpr();
+  assert(XCE && YCE && "ConceptDecl wihtout constraint expression?");
+  llvm::FoldingSetNodeID XID, YID;
+  XCE->Profile(XID, *this, /*Canonical=*/true);
+  YCE->Profile(YID, *this, /*Canonical=*/true);
+  if (XID != YID)
+return false;
+}
+
 return isSameEntity(TemplateX->getTemplatedDecl(),
 TemplateY->getTemplatedDecl()) &&
isSameTemplateParameterList(TemplateX->getTemplateParameters(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129104: [Modules] Add ODR Check for concepts

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 442179.
ChuanqiXu added a comment.

Undo the format test. It breaks the testing.


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

https://reviews.llvm.org/D129104

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/Modules/concept_differ.cpp
  clang/test/Modules/concept_differ.cppm

Index: clang/test/Modules/concept_differ.cppm
===
--- /dev/null
+++ clang/test/Modules/concept_differ.cppm
@@ -0,0 +1,39 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -x c++ -std=c++20 %t/A.cppm -I%t -emit-module-interface -o %t/A.pcm
+// RUN: %clang_cc1 -x c++ -std=c++20 %t/B.cppm -I%t -emit-module-interface -o %t/B.pcm
+// RUN: %clang_cc1 -x c++ -std=c++20 -fprebuilt-module-path=%t %t/foo.cpp -verify
+
+//--- foo.h
+template 
+concept A = true;
+
+//--- bar.h
+template 
+concept A = false;
+
+//--- A.cppm
+module;
+#include "foo.h"
+export module A;
+export using ::A;
+
+//--- B.cppm
+module;
+#include "bar.h"
+export module B;
+export using ::A;
+
+//--- foo.cpp
+import A;
+import B;
+
+template  void foo() requires A {}  // expected-error 1+{{reference to 'A' is ambiguous}}
+// expected-note@* 1+{{candidate found by name lookup}}
+
+int main() {
+  foo();
+  return 0;
+}
Index: clang/test/Modules/concept_differ.cpp
===
--- /dev/null
+++ clang/test/Modules/concept_differ.cpp
@@ -0,0 +1,35 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -x c++ -std=c++20 -fmodules -fmodules-cache-path=%t -fmodule-map-file=%t/module.map %t/foo.cpp -verify
+
+//--- module.map
+module "foo" {
+  export * 
+  header "foo.h"
+}
+module "bar" {
+  export * 
+  header "bar.h"
+}
+
+//--- foo.h
+template 
+concept A = true;
+
+//--- bar.h
+template 
+concept A = false;
+
+//--- foo.cpp
+#include "bar.h"
+#include "foo.h"
+
+template  void foo() requires A {}  // expected-error 1+{{reference to 'A' is ambiguous}}
+// expected-note@* 1+{{candidate found by name lookup}}
+
+int main() {
+  foo();
+  return 0;
+}
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6524,6 +6524,20 @@
   // and patterns match.
   if (const auto *TemplateX = dyn_cast(X)) {
 const auto *TemplateY = cast(Y);
+
+// ConceptDecl wouldn't be the same if their constraint expression differs.
+if (const auto *ConceptX = dyn_cast(X)) {
+  const auto *ConceptY = dyn_cast(Y);
+  const Expr *XCE = ConceptX->getConstraintExpr();
+  const Expr *YCE = ConceptY->getConstraintExpr();
+  assert(XCE && YCE && "ConceptDecl wihtout constraint expression?");
+  llvm::FoldingSetNodeID XID, YID;
+  XCE->Profile(XID, *this, /*Canonical=*/true);
+  YCE->Profile(YID, *this, /*Canonical=*/true);
+  if (XID != YID)
+return false;
+}
+
 return isSameEntity(TemplateX->getTemplatedDecl(),
 TemplateY->getTemplatedDecl()) &&
isSameTemplateParameterList(TemplateX->getTemplateParameters(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129068: [AST] Profiling on constraint expression instead of arguments for TypeConstraint in ASTContext::isSameTemplateParameter

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 442181.
ChuanqiXu added a comment.

Add tests for nested template parameters.


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

https://reviews.llvm.org/D129068

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/Modules/concept.cppm


Index: clang/test/Modules/concept.cppm
===
--- clang/test/Modules/concept.cppm
+++ clang/test/Modules/concept.cppm
@@ -18,6 +18,9 @@
 template 
 concept __member_size = requires(_Tp &&t) { t.size(); };
 
+template 
+concept C = requires(First x, Second y) { x+y; };
+
 struct A {
 public:
   template 
@@ -29,6 +32,16 @@
   constexpr __integer_like auto operator()(_Tp&& __t) const {
 return __t.size();
   }
+
+  template <__integer_like _Tp, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+return __t;
+  }
+
+  template  class H, class S, C> Sentinel>
+  constexpr H operator()(H &&__s, Sentinel &&last) const {
+return __s;
+  }
 };
 #endif
 
@@ -44,6 +57,11 @@
 export module B;
 import A;
 
+template 
+struct U {
+  auto operator+(U) { return 0; }
+};
+
 void foo() {
 A a;
 struct S {
@@ -51,4 +69,7 @@
 auto operator+(S s) { return 0; }
 };
 __fn{}(S());
+__fn{}(S(), S());
+
+__fn{}(U(), U());
 }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6245,14 +6245,29 @@
 auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
 if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
   return false;
-llvm::FoldingSetNodeID XID, YID;
-for (auto &ArgLoc : TXTCArgs->arguments())
-  ArgLoc.getArgument().Profile(XID, X->getASTContext());
-for (auto &ArgLoc : TYTCArgs->arguments())
-  ArgLoc.getArgument().Profile(YID, Y->getASTContext());
-if (XID != YID)
-  return false;
+// We couldn't compare the profiling result for the template
+// args here. Consider the following example in different modules:
+//
+// template <__integer_like _Tp, C<_Tp> Sentinel>
+// constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+//   return __t;
+// }
+//
+// When we compare the profiling result for `C<_Tp>` in different
+// modules, it will compare the type of `_Tp` in different modules.
+// However, the type of `_Tp` in different modules refer to different
+// types here naturally. So we couldn't compare the profiling result
+// for the template args directly.
   }
+  llvm::FoldingSetNodeID XID, YID;
+  auto *XConstraint = TXTC->getImmediatelyDeclaredConstraint();
+  auto *YConstraint = TYTC->getImmediatelyDeclaredConstraint();
+  if (XConstraint)
+XConstraint->Profile(XID, *this, /*Canonical=*/true);
+  if (YConstraint)
+YConstraint->Profile(YID, *this, /*Canonical=*/true);
+  if (XID != YID)
+return false;
 }
 return true;
   }


Index: clang/test/Modules/concept.cppm
===
--- clang/test/Modules/concept.cppm
+++ clang/test/Modules/concept.cppm
@@ -18,6 +18,9 @@
 template 
 concept __member_size = requires(_Tp &&t) { t.size(); };
 
+template 
+concept C = requires(First x, Second y) { x+y; };
+
 struct A {
 public:
   template 
@@ -29,6 +32,16 @@
   constexpr __integer_like auto operator()(_Tp&& __t) const {
 return __t.size();
   }
+
+  template <__integer_like _Tp, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+return __t;
+  }
+
+  template  class H, class S, C> Sentinel>
+  constexpr H operator()(H &&__s, Sentinel &&last) const {
+return __s;
+  }
 };
 #endif
 
@@ -44,6 +57,11 @@
 export module B;
 import A;
 
+template 
+struct U {
+  auto operator+(U) { return 0; }
+};
+
 void foo() {
 A a;
 struct S {
@@ -51,4 +69,7 @@
 auto operator+(S s) { return 0; }
 };
 __fn{}(S());
+__fn{}(S(), S());
+
+__fn{}(U(), U());
 }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6245,14 +6245,29 @@
 auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
 if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
   return false;
-llvm::FoldingSetNodeID XID, YID;
-for (auto &ArgLoc : TXTCArgs->arguments())
-  ArgLoc.getArgument().Profile(XID, X->getASTContext());
-for (auto &ArgLoc : TYTCArgs->arguments())
-  ArgLoc.getArgument().Profile(YID, Y->getASTContext());
-if (XID != YID)
-  return false;
+// We couldn't compare the profiling result for the template
+// args here. Consider the following exam

[PATCH] D129068: [AST] Profiling on constraint expression instead of arguments for TypeConstraint in ASTContext::isSameTemplateParameter

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D129068#3629135 , @vsapsai wrote:

> I don't know enough to say if it is a good approach or not, I'll need to 
> check what we can achieve by modifying `Profile` in 
> `ArgLoc.getArgument().Profile`.

I tried but I don't find good solution. If you could find something workable, 
it should be great.

> Specifically, I'm interested to see if we can use the same `ASTContext` for 
> profile.

I tried this but it doesn't work. They refer to the same `ASTContext`.

> Also I have a few stylistic comments but I want to figure out high-level 
> stuff first.

Of course.

> And I have some ideas about the tests. It might be covered elsewhere but I'm 
> curious if there are any not-isSameTemplateParameter test cases?

I am not sure what you mean about "not-isSameTemplateParameter test case".

> Also it can be useless but will it work with the deeper template nesting? 
> Something like `C>`.

Yeah, added.


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

https://reviews.llvm.org/D129068

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

[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

Ping @jyknight

Or anyone else will review this one? I want us to fix the thread problems in 
clang15, which would be created on July 26th.

@rjmccall @nhaehnle @danilaml @efriedma


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

https://reviews.llvm.org/D125291

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

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126694#3629251 , @iains wrote:

> In D126694#3629094 , @ChuanqiXu 
> wrote:
>
>> BTW, after I applied the patch, the compiler crashes at 
>> https://github.com/ChuanqiXu9/stdmodules.
>
> That link points to a project - is there (say) a gist of the crash 
> information?

Here is the crash log:

  unhandled type: 0x7cda9e0 LValueReferenceType 0x7cda9e0 'const struct 
std::_PairT &'
  `-QualType 0x7cd85f1 'const struct std::_PairT' const
`-RecordType 0x7cd85f0 'struct std::_PairT'
  `-CXXRecord 0x7cd8540 '_PairT'
  unhandled type: 0x7cdabb0 RValueReferenceType 0x7cdabb0 'struct std::_PairT 
&&'
  `-RecordType 0x7cd85f0 'struct std::_PairT'
`-CXXRecord 0x7cd8540 '_PairT'
  unhandled type: 0x80e0400 LValueReferenceType 0x80e0400 'struct std::_PairT &'
  `-RecordType 0x7cd85f0 'struct std::_PairT'
`-CXXRecord 0x7cd8540 '_PairT'
  unhandled type: 0x7c0e1a0 TemplateTypeParmType 0x7c0e1a0 '_T1' dependent 
depth 0 index 0
  `-TemplateTypeParm 0x7c0e148 '_T1'
  unhandled type: 0x7c0e220 TemplateTypeParmType 0x7c0e220 '_T2' dependent 
depth 0 index 1
  `-TemplateTypeParm 0x7c0e1d0 '_T2'
  unhandled type: 0x7c0e6a0 LValueReferenceType 0x7c0e6a0 'const pair<_T1, _T2> 
&' dependent
  `-QualType 0x7ace031 'const pair<_T1, _T2>' const
`-InjectedClassNameType 0x7ace030 'pair<_T1, _T2>' dependent
  `-CXXRecord 0x7c0e290 'pair'
  unhandled type: 0x7c0e8e0 RValueReferenceType 0x7c0e8e0 'pair<_T1, _T2> &&' 
dependent
  `-InjectedClassNameType 0x7ace030 'pair<_T1, _T2>' dependent
`-CXXRecord 0x7c0e290 'pair'
  clang++: 
/home/chuanqi.xcq/workspace.xuchuanqi/llvm-project-for-work/clang/lib/Sema/SemaModule.cpp:1084:
 void clang::Sema::markGMFDeclsReachableFrom(clang::Decl*, bool): Assertion 
`T->isRecordType() && "base not a record?"' failed.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
and include the crash backtrace, preprocessed source, and associated run script.
  Stack dump:
  0.Program arguments: clang++ -std=c++20 --precompile 
-fprebuilt-module-path=. coroutine.cppm -o std-coroutine.pcm -isystem 
../build_libcxx/include/c++/v1 
-I../build_libcxx/include/x86_64-unknown-linux-gnu/c++/v1 -nostdinc++
  1. parser at end of file
  2.../build_libcxx/include/c++/v1/__functional/hash.h:308:12: 
instantiating function definition 'std::__scalar_hash::operator()'
  3.../build_libcxx/include/c++/v1/__functional/hash.h:93:18: instantiating 
function definition 'std::__murmur2_or_cityhash::operator()'
   #0 0x01f60f90 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
   #1 0x01f5ede4 llvm::sys::CleanupOnSignal(unsigned long) 
(/disk2/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang-14+0x1f5ede4)
   #2 0x01eaa468 CrashRecoverySignalHandler(int) 
CrashRecoveryContext.cpp:0:0
   #3 0x77fb29d0 __restore_rt sigaction.c:0:0
   #4 0x77675f35 raise (/lib64/libc.so.6+0x3bf35)
   #5 0x7765f8d7 abort (/lib64/libc.so.6+0x258d7)
   #6 0x7765f7a7 _nl_load_domain.cold loadmsgcat.c:0:0
   #7 0x7766e536 (/lib64/libc.so.6+0x34536)
   #8 0x04696390 clang::Sema::markGMFDeclsReachableFrom(clang::Decl*, 
bool) 
(/disk2/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang-14+0x4696390)
   #9 0x04695d87 clang::Sema::markGMFDeclsReachableFrom(clang::Decl*, 
bool) 
(/disk2/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang-14+0x4695d87)
  #10 0x04696559 clang::Sema::findGMFReachableDeclsForType(clang::Type 
const*) 
(/disk2/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang-14+0x4696559)
  #11 0x0469689b clang::Sema::findGMFReachableDeclExprs(clang::Stmt*) 
(/disk2/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang-14+0x469689b)
  #12 0x046967bd clang::Sema::findGMFReachableDeclExprs(clang::Stmt*) 
(/disk2/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang-14+0x46967bd)
  #13 0x046967bd clang::Sema::findGMFReachableDeclExprs(clang::Stmt*) 
(/disk2/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang-14+0x46967bd)
  #14 0x046967bd clang::Sema::findGMFReachableDeclExprs(clang::Stmt*) 
(/disk2/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang-14+0x46967bd)
  #15 0x046967bd clang::Sema::findGMFReachableDeclExprs(clang::Stmt*) 
(/disk2/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang-14+0x46967bd)
  #16 0x046958c2 clang::Sema::markGMFDeclsReachableFrom(clang::Decl*, 
bool) 
(/disk2/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang-14+0x46958c2)
  #17 0x04695749 clang::Sema::markGMFDeclsReachableFrom(clang::Decl*, 
bool) 
(/disk2/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang-14+0x4695749)
  #18 0x04695e75 clang::Sema::markGMFDeclsReachableFrom(clang::Decl*, 
bool) 
(/disk2/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang-14+0x4695e75)
  

[PATCH] D129138: [clang] [docs] Update the changes of C++20 Modules in clang15

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu created this revision.
ChuanqiXu added reviewers: iains, aaron.ballman, erichkeane.
ChuanqiXu added a project: clang-language-wg.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Since clang15 is going to be branched in July 26, and C++ modules still lack an 
update on ReleaseNotes. Although it is not complete yet, I think it would be 
better to add one since we've done many works for C++20 Modules in clang15.

@iains would you like to check if the list is complete or not? I don't include 
D126694  since I feel like it might not be 
possible to be landed in 3 weeks. And I know there some other patches not 
covered, but I feel it might be chaos to our users.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129138

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -462,6 +462,12 @@
   that can be used for such compatibility. The demangler now demangles
   symbols with named module attachment.
 
+- Implemented some features for C++20 Modules, including: Partitions, 
+  Reachability, Header Unit and ``extern "C++"`` semantics.
+
+- Implemented `P1779R3: ABI isolation for member functions 
`_.
+- Implemented `P1874R1: Dynamic Initialization Order of Non-Local Variables in 
Modules `_.
+
 - As per "Conditionally Trivial Special Member Functions" (P0848), it is
   now possible to overload destructors using concepts. Note that the rest
   of the paper about other special member functions is not yet implemented.


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -462,6 +462,12 @@
   that can be used for such compatibility. The demangler now demangles
   symbols with named module attachment.
 
+- Implemented some features for C++20 Modules, including: Partitions, 
+  Reachability, Header Unit and ``extern "C++"`` semantics.
+
+- Implemented `P1779R3: ABI isolation for member functions `_.
+- Implemented `P1874R1: Dynamic Initialization Order of Non-Local Variables in Modules `_.
+
 - As per "Conditionally Trivial Special Member Functions" (P0848), it is
   now possible to overload destructors using concepts. Note that the rest
   of the paper about other special member functions is not yet implemented.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118034: [C++20] [Modules] Don't complain about duplicated default template argument across modules

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

Given @rsmith and @iains is busy and this revision is relatively small, 
innocent and looks good to me, I plan to land this in Friday in case there is 
no further comments.


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

https://reviews.llvm.org/D118034

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

[PATCH] D129104: [Modules] Add ODR Check for concepts

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 442425.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D129104

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/Modules/concept_differ.cpp
  clang/test/Modules/concept_differ.cppm

Index: clang/test/Modules/concept_differ.cppm
===
--- /dev/null
+++ clang/test/Modules/concept_differ.cppm
@@ -0,0 +1,39 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -x c++ -std=c++20 %t/A.cppm -I%t -emit-module-interface -o %t/A.pcm
+// RUN: %clang_cc1 -x c++ -std=c++20 %t/B.cppm -I%t -emit-module-interface -o %t/B.pcm
+// RUN: %clang_cc1 -x c++ -std=c++20 -fprebuilt-module-path=%t %t/foo.cpp -verify
+
+//--- foo.h
+template 
+concept A = true;
+
+//--- bar.h
+template 
+concept A = false;
+
+//--- A.cppm
+module;
+#include "foo.h"
+export module A;
+export using ::A;
+
+//--- B.cppm
+module;
+#include "bar.h"
+export module B;
+export using ::A;
+
+//--- foo.cpp
+import A;
+import B;
+
+template  void foo() requires A {}  // expected-error 1+{{reference to 'A' is ambiguous}}
+// expected-note@* 1+{{candidate found by name lookup}}
+
+int main() {
+  foo();
+  return 0;
+}
Index: clang/test/Modules/concept_differ.cpp
===
--- /dev/null
+++ clang/test/Modules/concept_differ.cpp
@@ -0,0 +1,35 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -x c++ -std=c++20 -fmodules -fmodules-cache-path=%t -fmodule-map-file=%t/module.map %t/foo.cpp -verify
+
+//--- module.map
+module "foo" {
+  export * 
+  header "foo.h"
+}
+module "bar" {
+  export * 
+  header "bar.h"
+}
+
+//--- foo.h
+template 
+concept A = true;
+
+//--- bar.h
+template 
+concept A = false;
+
+//--- foo.cpp
+#include "bar.h"
+#include "foo.h"
+
+template  void foo() requires A {}  // expected-error 1+{{reference to 'A' is ambiguous}}
+// expected-note@* 1+{{candidate found by name lookup}}
+
+int main() {
+  foo();
+  return 0;
+}
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6524,6 +6524,20 @@
   // and patterns match.
   if (const auto *TemplateX = dyn_cast(X)) {
 const auto *TemplateY = cast(Y);
+
+// ConceptDecl wouldn't be the same if their constraint expression differs.
+if (const auto *ConceptX = dyn_cast(X)) {
+  const auto *ConceptY = cast(Y);
+  const Expr *XCE = ConceptX->getConstraintExpr();
+  const Expr *YCE = ConceptY->getConstraintExpr();
+  assert(XCE && YCE && "ConceptDecl without constraint expression?");
+  llvm::FoldingSetNodeID XID, YID;
+  XCE->Profile(XID, *this, /*Canonical=*/true);
+  YCE->Profile(YID, *this, /*Canonical=*/true);
+  if (XID != YID)
+return false;
+}
+
 return isSameEntity(TemplateX->getTemplatedDecl(),
 TemplateY->getTemplatedDecl()) &&
isSameTemplateParameterList(TemplateX->getTemplateParameters(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129104: [Modules] Add ODR Check for concepts

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:6532
+  const Expr *XCE = ConceptX->getConstraintExpr();
+  const Expr *YCE = ConceptY->getConstraintExpr();
+  assert(XCE && YCE && "ConceptDecl wihtout constraint expression?");

erichkeane wrote:
> This isn't necessarily valid here, you did a dyn_cast above.
Oh, my bad. I should use `cast`. We could find the style at line 6525-6526 and 
line 6519-6520.


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

https://reviews.llvm.org/D129104

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

[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a reviewer: nikic.
ChuanqiXu marked 2 inline comments as done.
ChuanqiXu added a comment.

In D125291#3629276 , @nikic wrote:

> FWIW the bitcode patch has landed, so implementing the variant using a token 
> type should be possible now. I'm not sure whether it's better to start with 
> the current patch where the intrinsic is optional, or go directly to the one 
> where it is required.

I preferred to start with the current patch. Since my original idea is to fix 
the thread problems in coroutines and @jyknight said we could fix the two 
problems in one approach. But I still want to fix the thread problems in 
coroutines first.

---

A big problem I meet now is about the constant expression would merge into the 
argument automatically so the verifier would fail. Here is the example:

  C++
  union U {
int a;
float f;
constexpr U() : f(0.0) {}
  };
  static thread_local U u;
  void *p = &u;

Then it would generate following code for `u`:

  define internal void @__cxx_global_var_init() #0 section ".text.startup" {
  entry:
%0 = call %union.U* @llvm.threadlocal.address.p0s_union.Us(%union.U* 
bitcast ({ float }* @_ZL1u to %union.U*))
%1 = bitcast %union.U* %0 to i8*
store i8* %1, i8** @p, align 8
ret void
  }

Then the verifier would complain since the argument of 
`@llvm.threadlocal.address` is not theadlocal global value. And I feel this 
might not be a simple problem to fix. I feel like we could only fix it after we 
made https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179. 
But it should be a long term goal to me.

---

So now it looks like there are two options:

1. Remove the verifier part.
2. Wait until we removed most constant expressions. In this case, I could only 
to pick up original methods to solve the thread problems in coroutines.

@jyknight @nikic  @nhaehnle @efriedma @rjmccall How do you think about this?




Comment at: llvm/docs/LangRef.rst:24415
+
+  declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
+

nikic wrote:
> Don't we need to overload this intrinsic by return type, so it works with 
> different address spaces?
Done. So we could remove the limit for opaque pointers too. So now it covers 
more tests.



Comment at: llvm/docs/LangRef.rst:24420
+
+The first argument is pointer, which refers to a thread local variable.
+

nikic wrote:
> Should we enforce here (with a verifier check) that the argument is a 
> thread-local global variable? I assume we //don't// want to allow weird 
> things like `@llvm.threadlocal.address(c ? @g1 : @g2)` right? (Though I 
> guess, without thread-local globals using a token type, nothing would prevent 
> optimizations from forming this.)
I originally added assertions when creating the intrinsics. But I add a check 
in the verifier in this revision.


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

https://reviews.llvm.org/D125291

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

[PATCH] D129174: [C++20][Modules] Invalidate internal-linkage functions in overload sets [P1815R2 part 1]

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/include/clang/Sema/Overload.h:800
+/// This candidate was not viable because it has internal linkage and is
+/// from a different module than the use.
+ovl_fail_module_mismatched,





Comment at: clang/lib/Sema/SemaOverload.cpp:6403
 
+  // Functions with internal linkage are only viable in the same module.
+  if (auto *MF = Function->getOwningModule()) {





Comment at: clang/lib/Sema/SemaOverload.cpp:6406
+if (Function->getFormalLinkage() <= Linkage::InternalLinkage &&
+getLangOpts().CPlusPlus20 && MF != getCurrentModule()) {
+  Candidate.Viable = false;

The current implementation may reject following two cases:
```
module;
static void foo(int); // Ignore header
...
export module A;
void bar() { foo(5); } // Should it be invalid?
```
and
```
export module A;
static void foo(int);
...
module :private;
void bar() { foo(5); } // Should it be invalid?
```

I mean in the above examples, the function of `foo(int)` is defined in the same 
TU but the call to it might be rejected.



Comment at: clang/test/CXX/basic/basic.link/p10-ex2.cpp:10-35
+//--- decls.h
+int f(); // #1, attached to the global module
+int g(); // #2, attached to the global module
+
+//--- M.cpp
+module;
+#include "decls.h"

I feel like the test is irrelevant with the revision, isn't it? If yes, I think 
we could land this as a separate NFC patch.



Comment at: 
clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p5-ex2.cpp:36-41
+//--- Q.cpp
+export module Q;
+
+//--- Q-impl.cpp
+module Q;
+import N;

This looks more consistent with the example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129174

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

[PATCH] D129138: [clang] [docs] Update the changes of C++20 Modules in clang15

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 442487.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D129138

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -466,6 +466,14 @@
   that can be used for such compatibility. The demangler now demangles
   symbols with named module attachment.
 
+- Enhanced the support for C++20 Modules, including: Partitions,
+  Reachability, Header Unit and ``extern "C++"`` semantics.
+
+- Implemented `P1103R3: Merging Modules `_.
+- Implemented `P1779R3: ABI isolation for member functions 
`_.
+- Implemented `P1874R1: Dynamic Initialization Order of Non-Local Variables in 
Modules `_.
+- Partially implemented `P1815R2: Translation-unit-local entities 
`_.
+
 - As per "Conditionally Trivial Special Member Functions" (P0848), it is
   now possible to overload destructors using concepts. Note that the rest
   of the paper about other special member functions is not yet implemented.


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -466,6 +466,14 @@
   that can be used for such compatibility. The demangler now demangles
   symbols with named module attachment.
 
+- Enhanced the support for C++20 Modules, including: Partitions,
+  Reachability, Header Unit and ``extern "C++"`` semantics.
+
+- Implemented `P1103R3: Merging Modules `_.
+- Implemented `P1779R3: ABI isolation for member functions `_.
+- Implemented `P1874R1: Dynamic Initialization Order of Non-Local Variables in Modules `_.
+- Partially implemented `P1815R2: Translation-unit-local entities `_.
+
 - As per "Conditionally Trivial Special Member Functions" (P0848), it is
   now possible to overload destructors using concepts. Note that the rest
   of the paper about other special member functions is not yet implemented.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129138: [clang] [docs] Update the changes of C++20 Modules in clang15

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D129138#3631912 , @iains wrote:

> Perhaps we could be a little more bold about the completeness of the 
> implementation (at least, w.r.t the base paper `P1103`) - we pass the 
> relevant test cases.

I added the wording like `Implemented `P1103R3`. I am not sure if it is good to 
say this. We made some progress indeed. But both of us know there are some 
FIXME remains.

> As for the follow-on papers, I think we have more that can be added notes 
> below:
> There are some test cases to be posted to phab for some of these (so maybe 
> allow me a few more days to get the list fully correct).
>
> (it will also depend on what we can land before 26th - however some of the 
> stuff below is already approved, so it's a matter of finding some time to 
> push the patches and watch the bots...)
>
> @ChuanqiXu if you think that more is needed on any of these (other than 
> `P1815` which is known partial), please let me know.
>
> (Please also add any relevant phab reviews from your side)
>
> -
>
> P1779R3: ABI isolation for member functions
>
> - Paper applied to WP.
>
> Change [dcl.inline]/7 (as edited by P1815R1):
>
> - This is being addressed by D128328 
>
> (although we have somewhat of a moving target since some clarifications were 
> requested from core).
>
> Change [dcl.fct.def.default]/3:
>
> - Already handled in the constexpr/consteval code, there is no actual change 
> for modular cases.
>
> Change [class.mfct]/1:
> Change [class.friend]/7:
>
> - D129045 , including tests.
>
> --
>
> P1979R0: Resolution to US086
>
> - Paper applied to WP.
> - Current implementation complies: test case to be posted 
> CXX/module/module.import/p7.cpp
>
> --
>
> P1811R0 Relaxing redefinition restrictions for re-exportation robustness
>
> - Paper applied to WP - note there are on-going discussions in core/ext about 
> exports that might affect this.
>
> Note that there are a lot of changes here, but that the draft that includes 
> them is what we are working to, so it is expected that (mostly) we will need 
> to identify tests and/or queries about how to verify.
>
> Change in 6.2 [basic.def.odr] paragraph 1:
> Change in 6.2 [basic.def.odr] paragraph 12:
>
> - no action required, these changes restore pre-P1103 behaviour (and the 
> paragraph 12 changes have been subsumed in following updates).
>
> Change in 10.5 [module.context] paragraph 7:
>
>   test: CXX/module/module.context/p7.cpp
>
> Change in 10.6 [module.reach] paragraph 3:
>
> - D126694  and D128328 
> 
>
> Change in 15.2 [cpp.include] paragraph 7:
>
> - D128981  provides conditional include 
> translation that follows the same scheme as GCC.
>
> Feature test macro
>
> - already implemented.
>
> ---
>
> [Partial] P1815R2: Translation-unit-local entities
>
> Change [basic.def.odr]/10:
>
> - implementation complies, example provided.
>
> Insert before [basic.def.odr]/11
>
> - D128328  + ongoing core/ext discussions.
>
> Change [basic.lookup.argdep]/5:
> D129174  - overload changes.  example added 
>  Question to core on TU#3  f(x).
>
> ---
>
> P2115R0: US069: Merging of multiple definitions for unnamed unscoped 
> enumerations
>
> - merged to WP, but ongoing discussions in core/ext might cause re-work
>
> testcase to be posted to phab.

Thanks for the summary! But I am afraid it is too fine-grained to users. It 
looks like the current ReleaseNotes don't contain phab links nor new added test 
cases. @erichkeane @aaron.ballman may you give some advice?


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

https://reviews.llvm.org/D129138

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

[PATCH] D129138: [clang] [docs] Update the changes of C++20 Modules in clang15

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 442761.
ChuanqiXu added a comment.

Update cxx_status.html


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

https://reviews.llvm.org/D129138

Files:
  clang/docs/ReleaseNotes.rst
  clang/www/cxx_status.html


Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1164,7 +1164,7 @@
 
   Modules
   https://wg21.link/p1103r3";>P1103R3
-  Partial
+  Clang 15
 

 https://wg21.link/p1766r1";>P1766R1 (DR)
@@ -1180,17 +1180,19 @@
   

 https://wg21.link/p1874r1";>P1874R1
-Partial
+Clang 15
   

 https://wg21.link/p1979r0";>P1979R0
-No
+No
   

 https://wg21.link/p1779r3";>P1779R3
+Clang 15
   
   
 https://wg21.link/p1857r3";>P1857R3
+No
   
   
 https://wg21.link/p2115r0";>P2115R0
@@ -1198,7 +1200,7 @@
   
   
 https://wg21.link/p1815r2";>P1815R2
-No
+Partial
   
 
   Coroutines
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -466,6 +466,14 @@
   that can be used for such compatibility. The demangler now demangles
   symbols with named module attachment.
 
+- Enhanced the support for C++20 Modules, including: Partitions,
+  Reachability, Header Unit and ``extern "C++"`` semantics.
+
+- Implemented `P1103R3: Merging Modules `_.
+- Implemented `P1779R3: ABI isolation for member functions 
`_.
+- Implemented `P1874R1: Dynamic Initialization Order of Non-Local Variables in 
Modules `_.
+- Partially implemented `P1815R2: Translation-unit-local entities 
`_.
+
 - As per "Conditionally Trivial Special Member Functions" (P0848), it is
   now possible to overload destructors using concepts. Note that the rest
   of the paper about other special member functions is not yet implemented.


Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1164,7 +1164,7 @@
 
   Modules
   https://wg21.link/p1103r3";>P1103R3
-  Partial
+  Clang 15
 

 https://wg21.link/p1766r1";>P1766R1 (DR)
@@ -1180,17 +1180,19 @@
   

 https://wg21.link/p1874r1";>P1874R1
-Partial
+Clang 15
   

 https://wg21.link/p1979r0";>P1979R0
-No
+No
   

 https://wg21.link/p1779r3";>P1779R3
+Clang 15
   
   
 https://wg21.link/p1857r3";>P1857R3
+No
   
   
 https://wg21.link/p2115r0";>P2115R0
@@ -1198,7 +1200,7 @@
   
   
 https://wg21.link/p1815r2";>P1815R2
-No
+Partial
   
 
   Coroutines
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -466,6 +466,14 @@
   that can be used for such compatibility. The demangler now demangles
   symbols with named module attachment.
 
+- Enhanced the support for C++20 Modules, including: Partitions,
+  Reachability, Header Unit and ``extern "C++"`` semantics.
+
+- Implemented `P1103R3: Merging Modules `_.
+- Implemented `P1779R3: ABI isolation for member functions `_.
+- Implemented `P1874R1: Dynamic Initialization Order of Non-Local Variables in Modules `_.
+- Partially implemented `P1815R2: Translation-unit-local entities `_.
+
 - As per "Conditionally Trivial Special Member Functions" (P0848), it is
   now possible to overload destructors using concepts. Note that the rest
   of the paper about other special member functions is not yet implemented.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129138: [clang] [docs] Update the changes of C++20 Modules in clang15

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked an inline comment as done.
ChuanqiXu added a comment.

In D129138#3632128 , @iains wrote:

> I would not expect to add all this information to the release notes, or any 
> of the phab links - just single lines to say that paper numbers are 
> implemented
>
> - the details are just to help us track the situation up to 26th July.

Got it. Thanks!




Comment at: clang/docs/ReleaseNotes.rst:472-476
+- Implemented `P1103R3: Merging Modules `_.
+- Implemented `P1779R3: ABI isolation for member functions 
`_.
+- Implemented `P1874R1: Dynamic Initialization Order of Non-Local Variables in 
Modules `_.
+- Partially implemented `P1815R2: Translation-unit-local entities 
`_.
+

h-vetinari wrote:
> This should probably also be reflected in 
> https://clang.llvm.org/cxx_status.html / 
> https://github.com/llvm/llvm-project/blob/main/clang/www/cxx_status.html ...?
Done. Thanks!


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

https://reviews.llvm.org/D129138

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

[PATCH] D129174: [C++20][Modules] Invalidate internal-linkage functions in overload sets [P1815R2 part 1]

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:6406
+if (Function->getFormalLinkage() <= Linkage::InternalLinkage &&
+getLangOpts().CPlusPlus20 && MF != getCurrentModule()) {
+  Candidate.Viable = false;

iains wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > The current implementation may reject following two cases:
> > > ```
> > > module;
> > > static void foo(int); // Ignore header
> > > ...
> > > export module A;
> > > void bar() { foo(5); } // Should it be invalid?
> > > ```
> > > and
> > > ```
> > > export module A;
> > > static void foo(int);
> > > ...
> > > module :private;
> > > void bar() { foo(5); } // Should it be invalid?
> > > ```
> > > 
> > > I mean in the above examples, the function of `foo(int)` is defined in 
> > > the same TU but the call to it might be rejected.
> > > The current implementation may reject following two cases:
> > > ```
> > > module;
> > > static void foo(int); // Ignore header
> > > ...
> > > export module A;
> > > void bar() { foo(5); } // Should it be invalid?
> > > ```
> > > and
> > > ```
> > > export module A;
> > > static void foo(int);
> > > ...
> > > module :private;
> > > void bar() { foo(5); } // Should it be invalid?
> > > ```
> > > 
> > > I mean in the above examples, the function of `foo(int)` is defined in 
> > > the same TU but the call to it might be rejected.
> > 
> > 
> > The current implementation may reject following two cases:
> > ```
> > module;
> > static void foo(int); // Ignore header
> > ...
> 
> Actually, I have a note to check on the global module case, since we have 
> special rules that allow merging of items there,
> 
> 
> > export module A;
> > void bar() { foo(5); } // Should it be invalid?
> > ```
> > and
> > ```
> > export module A;
> > static void foo(int);
> > ...
> > module :private;
> > void bar() { foo(5); } // Should it be invalid?
> > ```
> > 
> > I mean in the above examples, the function of `foo(int)` is defined in the 
> > same TU but the call to it might be rejected.
> 
> otherwise, agreed, these cases should be ok -  I guess we need a second test 
> case with lookups that should succeed.
> 
> Actually, I have a note to check on the global module case, since we have 
> special rules that allow merging of items there,

Yeah, it is surprising that we couldn't handle the global module fragment case.



Comment at: 
clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p5-ex2.cpp:36-41
+//--- Q.cpp
+export module Q;
+
+//--- Q-impl.cpp
+module Q;
+import N;

iains wrote:
> ChuanqiXu wrote:
> > This looks more consistent with the example.
> This changes the example so that M is directly included in the implementation 
> rather than transitively (the example does specifically use a different 
> module name for the implementation)
> 
> I am not sure what you mean by "more consistent"
>  (the example will fail to reject some of the lookups with this change).
> 
> 
> 
Oh, I didn't notice it uses a different module name. My bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129174

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

[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Sema/SemaTemplate.cpp:8679
+  Previous.isSingleResult() ? Previous.getAsSingle() : 
nullptr;
+  if (PrevDef && !hasVisibleDefinition(PrevDef) &&
+  Context.isSameEntity(NewDecl, PrevDef)) {

rsmith wrote:
> ChuanqiXu wrote:
> > 
> I don't think that's right: [the C++20 modules 
> rule](http://eel.is/c++draft/basic.def.odr#2) is that at most one definition 
> is permitted per translation unit, and the Clang header modules rule is that 
> a definition is only permissible if no definition is visible (Clang header 
> modules effectively behave as if they're part of the same translation unit 
> for the purposes of this check). For now, checking for a visible definition 
> seems better than checking for a reachable one, until we implement the C++20 
> rule in general.
For clang modules, the reachability should be the visibility: 
https://github.com/llvm/llvm-project/blob/9cb00e7133ea3da6a49ade95e3708240c2aaae39/clang/lib/Sema/SemaLookup.cpp#L1903-L1905.
 So I think it is better to check for a reachable definition here. Otherwise we 
might not be able to handle the following case:
```
import M;
export template 
concept ConflictingConcept = true; // There is another reachable but not 
visible definition for ConflictingConcept.
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128921

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

[PATCH] D128974: [AST] [Modules] Handle full cases of DefaultArgStorage::setInherited

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 442791.
ChuanqiXu added a comment.

Rebase


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

https://reviews.llvm.org/D128974

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/DeclTemplate.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Modules/InheritDefaultArguments.cppm
  clang/test/Modules/Inputs/PR31469/textual.h

Index: clang/test/Modules/Inputs/PR31469/textual.h
===
--- clang/test/Modules/Inputs/PR31469/textual.h
+++ clang/test/Modules/Inputs/PR31469/textual.h
@@ -4,7 +4,7 @@
   template  class allocator;
   template > class list;
   template  class __list_iterator {
-//template  friend class list; // causes another crash in ASTDeclReader::attachPreviousDecl
+template  friend class list;
 template  friend class list;
   };
   template  class __list_imp {};
Index: clang/test/Modules/InheritDefaultArguments.cppm
===
--- clang/test/Modules/InheritDefaultArguments.cppm
+++ clang/test/Modules/InheritDefaultArguments.cppm
@@ -2,8 +2,8 @@
 // RUN: split-file %s %t
 // RUN: cd %t
 //
-// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm
-// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -I%t %t/Use.cppm -verify -fsyntax-only
+// RUN: %clang -std=c++20 %t/A.cppm --precompile -o %t/A.pcm
+// RUN: %clang -std=c++20 -fprebuilt-module-path=%t -I%t %t/Use.cppm -Xclang -verify -S -o -
 
 //--- foo.h
 template 
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -2643,46 +2643,6 @@
   return false;
 }
 
-static bool hasSameDefaultTemplateArgument(NamedDecl *X, NamedDecl *Y) {
-  ASTContext &C = X->getASTContext();
-  // If the type parameter isn't the same already, we don't need to check the
-  // default argument further.
-  if (!C.isSameTemplateParameter(X, Y))
-return false;
-
-  if (auto *TTPX = dyn_cast(X)) {
-auto *TTPY = cast(Y);
-if (!TTPX->hasDefaultArgument() || !TTPY->hasDefaultArgument())
-  return false;
-
-return C.hasSameType(TTPX->getDefaultArgument(),
- TTPY->getDefaultArgument());
-  }
-
-  if (auto *NTTPX = dyn_cast(X)) {
-auto *NTTPY = cast(Y);
-if (!NTTPX->hasDefaultArgument() || !NTTPY->hasDefaultArgument())
-  return false;
-
-Expr *DefaultArgumentX = NTTPX->getDefaultArgument()->IgnoreImpCasts();
-Expr *DefaultArgumentY = NTTPY->getDefaultArgument()->IgnoreImpCasts();
-llvm::FoldingSetNodeID XID, YID;
-DefaultArgumentX->Profile(XID, C, /*Canonical=*/true);
-DefaultArgumentY->Profile(YID, C, /*Canonical=*/true);
-return XID == YID;
-  }
-
-  auto *TTPX = cast(X);
-  auto *TTPY = cast(Y);
-
-  if (!TTPX->hasDefaultArgument() || !TTPY->hasDefaultArgument())
-return false;
-
-  const TemplateArgument &TAX = TTPX->getDefaultArgument().getArgument();
-  const TemplateArgument &TAY = TTPY->getDefaultArgument().getArgument();
-  return C.hasSameTemplateName(TAX.getAsTemplate(), TAY.getAsTemplate());
-}
-
 /// Checks the validity of a template parameter list, possibly
 /// considering the template parameter list from a previous
 /// declaration.
@@ -2780,7 +2740,8 @@
 if (!OldTypeParm->getOwningModule() ||
 isModuleUnitOfCurrentTU(OldTypeParm->getOwningModule()))
   RedundantDefaultArg = true;
-else if (!hasSameDefaultTemplateArgument(OldTypeParm, NewTypeParm)) {
+else if (!getASTContext().isSameDefaultTemplateArgument(OldTypeParm,
+NewTypeParm)) {
   InconsistentDefaultArg = true;
   PrevModuleName =
   OldTypeParm->getImportedOwningModule()->getFullModuleName();
@@ -2832,8 +2793,8 @@
 if (!OldNonTypeParm->getOwningModule() ||
 isModuleUnitOfCurrentTU(OldNonTypeParm->getOwningModule()))
   RedundantDefaultArg = true;
-else if (!hasSameDefaultTemplateArgument(OldNonTypeParm,
- NewNonTypeParm)) {
+else if (!getASTContext().isSameDefaultTemplateArgument(
+ OldNonTypeParm, NewNonTypeParm)) {
   InconsistentDefaultArg = true;
   PrevModuleName =
   OldNonTypeParm->getImportedOwningModule()->getFullModuleName();
@@ -2884,8 +2845,8 @@
 if (!OldTemplateParm->getOwningModule() ||
 isModuleUnitOfCurrentTU(OldTemplateParm->getOwningModule()))
   RedundantDefaultArg = true;
-else if (!hasSameDefaultTemplateArgument(OldTemplateParm,
- NewTemplateParm)) {
+else if (!getASTContext().isSameDefaultTemplateArgument(
+ OldTemplateParm, NewTemplateParm

[PATCH] D129068: [AST] Profiling on constraint expression instead of arguments for TypeConstraint in ASTContext::isSameTemplateParameter

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 442792.
ChuanqiXu added a comment.

Minor changes.


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

https://reviews.llvm.org/D129068

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/Modules/concept.cppm


Index: clang/test/Modules/concept.cppm
===
--- clang/test/Modules/concept.cppm
+++ clang/test/Modules/concept.cppm
@@ -18,6 +18,9 @@
 template 
 concept __member_size = requires(_Tp &&t) { t.size(); };
 
+template 
+concept C = requires(First x, Second y) { x+y; };
+
 struct A {
 public:
   template 
@@ -29,6 +32,16 @@
   constexpr __integer_like auto operator()(_Tp&& __t) const {
 return __t.size();
   }
+
+  template <__integer_like _Tp, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+return __t;
+  }
+
+  template  class H, class S, C> Sentinel>
+  constexpr H operator()(H &&__s, Sentinel &&last) const {
+return __s;
+  }
 };
 #endif
 
@@ -44,6 +57,11 @@
 export module B;
 import A;
 
+template 
+struct U {
+  auto operator+(U) { return 0; }
+};
+
 void foo() {
 A a;
 struct S {
@@ -51,4 +69,7 @@
 auto operator+(S s) { return 0; }
 };
 __fn{}(S());
+__fn{}(S(), S());
+
+__fn{}(U(), U());
 }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6245,14 +6245,31 @@
 auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
 if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
   return false;
-llvm::FoldingSetNodeID XID, YID;
-for (auto &ArgLoc : TXTCArgs->arguments())
-  ArgLoc.getArgument().Profile(XID, X->getASTContext());
-for (auto &ArgLoc : TYTCArgs->arguments())
-  ArgLoc.getArgument().Profile(YID, Y->getASTContext());
-if (XID != YID)
-  return false;
+// We couldn't compare the profiling result for the template
+// args here. Consider the following example in different modules:
+//
+// template <__integer_like _Tp, C<_Tp> Sentinel>
+// constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+//   return __t;
+// }
+//
+// When we compare the profiling result for `C<_Tp>` in different
+// modules, it will compare the type of `_Tp` in different modules.
+// However, the type of `_Tp` in different modules refer to different
+// types here naturally. So we couldn't compare the profiling result
+// for the template args directly.
+  }
+  llvm::FoldingSetNodeID XID, YID;
+  auto *XConstraint = TXTC->getImmediatelyDeclaredConstraint();
+  auto *YConstraint = TYTC->getImmediatelyDeclaredConstraint();
+  if (!XConstraint != !YConstraint)
+return false;
+  if (XConstraint) {
+XConstraint->Profile(XID, *this, /*Canonical=*/true);
+YConstraint->Profile(YID, *this, /*Canonical=*/true);
   }
+  if (XID != YID)
+return false;
 }
 return true;
   }


Index: clang/test/Modules/concept.cppm
===
--- clang/test/Modules/concept.cppm
+++ clang/test/Modules/concept.cppm
@@ -18,6 +18,9 @@
 template 
 concept __member_size = requires(_Tp &&t) { t.size(); };
 
+template 
+concept C = requires(First x, Second y) { x+y; };
+
 struct A {
 public:
   template 
@@ -29,6 +32,16 @@
   constexpr __integer_like auto operator()(_Tp&& __t) const {
 return __t.size();
   }
+
+  template <__integer_like _Tp, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+return __t;
+  }
+
+  template  class H, class S, C> Sentinel>
+  constexpr H operator()(H &&__s, Sentinel &&last) const {
+return __s;
+  }
 };
 #endif
 
@@ -44,6 +57,11 @@
 export module B;
 import A;
 
+template 
+struct U {
+  auto operator+(U) { return 0; }
+};
+
 void foo() {
 A a;
 struct S {
@@ -51,4 +69,7 @@
 auto operator+(S s) { return 0; }
 };
 __fn{}(S());
+__fn{}(S(), S());
+
+__fn{}(U(), U());
 }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6245,14 +6245,31 @@
 auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
 if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
   return false;
-llvm::FoldingSetNodeID XID, YID;
-for (auto &ArgLoc : TXTCArgs->arguments())
-  ArgLoc.getArgument().Profile(XID, X->getASTContext());
-for (auto &ArgLoc : TYTCArgs->arguments())
-  ArgLoc.getArgument().Profile(YID, Y->getASTContext());
-if (XID != YID)
-  return false;
+// We couldn't compare the profiling result for the template
+// args here. Con

[PATCH] D128974: [AST] [Modules] Handle full cases of DefaultArgStorage::setInherited

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 442795.
ChuanqiXu added a comment.

Update tests.


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

https://reviews.llvm.org/D128974

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/DeclTemplate.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Modules/InheritDefaultArguments.cppm
  clang/test/Modules/Inputs/PR31469/textual.h

Index: clang/test/Modules/Inputs/PR31469/textual.h
===
--- clang/test/Modules/Inputs/PR31469/textual.h
+++ clang/test/Modules/Inputs/PR31469/textual.h
@@ -4,7 +4,7 @@
   template  class allocator;
   template > class list;
   template  class __list_iterator {
-//template  friend class list; // causes another crash in ASTDeclReader::attachPreviousDecl
+template  friend class list;
 template  friend class list;
   };
   template  class __list_imp {};
Index: clang/test/Modules/InheritDefaultArguments.cppm
===
--- clang/test/Modules/InheritDefaultArguments.cppm
+++ clang/test/Modules/InheritDefaultArguments.cppm
@@ -10,7 +10,10 @@
 class Templ;
 
 template 
-class Templ {};
+class Templ {
+public:
+Templ(T t) {}
+};
 
 template 
 Templ(T t) -> Templ;
@@ -26,3 +29,6 @@
 #include "foo.h"
 export module X;
 import A;
+void foo() {
+Templ t(0);
+}
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -2643,46 +2643,6 @@
   return false;
 }
 
-static bool hasSameDefaultTemplateArgument(NamedDecl *X, NamedDecl *Y) {
-  ASTContext &C = X->getASTContext();
-  // If the type parameter isn't the same already, we don't need to check the
-  // default argument further.
-  if (!C.isSameTemplateParameter(X, Y))
-return false;
-
-  if (auto *TTPX = dyn_cast(X)) {
-auto *TTPY = cast(Y);
-if (!TTPX->hasDefaultArgument() || !TTPY->hasDefaultArgument())
-  return false;
-
-return C.hasSameType(TTPX->getDefaultArgument(),
- TTPY->getDefaultArgument());
-  }
-
-  if (auto *NTTPX = dyn_cast(X)) {
-auto *NTTPY = cast(Y);
-if (!NTTPX->hasDefaultArgument() || !NTTPY->hasDefaultArgument())
-  return false;
-
-Expr *DefaultArgumentX = NTTPX->getDefaultArgument()->IgnoreImpCasts();
-Expr *DefaultArgumentY = NTTPY->getDefaultArgument()->IgnoreImpCasts();
-llvm::FoldingSetNodeID XID, YID;
-DefaultArgumentX->Profile(XID, C, /*Canonical=*/true);
-DefaultArgumentY->Profile(YID, C, /*Canonical=*/true);
-return XID == YID;
-  }
-
-  auto *TTPX = cast(X);
-  auto *TTPY = cast(Y);
-
-  if (!TTPX->hasDefaultArgument() || !TTPY->hasDefaultArgument())
-return false;
-
-  const TemplateArgument &TAX = TTPX->getDefaultArgument().getArgument();
-  const TemplateArgument &TAY = TTPY->getDefaultArgument().getArgument();
-  return C.hasSameTemplateName(TAX.getAsTemplate(), TAY.getAsTemplate());
-}
-
 /// Checks the validity of a template parameter list, possibly
 /// considering the template parameter list from a previous
 /// declaration.
@@ -2780,7 +2740,8 @@
 if (!OldTypeParm->getOwningModule() ||
 isModuleUnitOfCurrentTU(OldTypeParm->getOwningModule()))
   RedundantDefaultArg = true;
-else if (!hasSameDefaultTemplateArgument(OldTypeParm, NewTypeParm)) {
+else if (!getASTContext().isSameDefaultTemplateArgument(OldTypeParm,
+NewTypeParm)) {
   InconsistentDefaultArg = true;
   PrevModuleName =
   OldTypeParm->getImportedOwningModule()->getFullModuleName();
@@ -2832,8 +2793,8 @@
 if (!OldNonTypeParm->getOwningModule() ||
 isModuleUnitOfCurrentTU(OldNonTypeParm->getOwningModule()))
   RedundantDefaultArg = true;
-else if (!hasSameDefaultTemplateArgument(OldNonTypeParm,
- NewNonTypeParm)) {
+else if (!getASTContext().isSameDefaultTemplateArgument(
+ OldNonTypeParm, NewNonTypeParm)) {
   InconsistentDefaultArg = true;
   PrevModuleName =
   OldNonTypeParm->getImportedOwningModule()->getFullModuleName();
@@ -2884,8 +2845,8 @@
 if (!OldTemplateParm->getOwningModule() ||
 isModuleUnitOfCurrentTU(OldTemplateParm->getOwningModule()))
   RedundantDefaultArg = true;
-else if (!hasSameDefaultTemplateArgument(OldTemplateParm,
- NewTemplateParm)) {
+else if (!getASTContext().isSameDefaultTemplateArgument(
+ OldTemplateParm, NewTemplateParm)) {
   InconsistentDefaultArg = true;
   PrevModuleName =
   OldTemplateParm->getImportedOwningModule()->getFullModuleName();
Index: cl

[PATCH] D129279: [NFC] [Coroutines] Add regression test for heap allocation elision optimization

[Chuanqi Xu via Phabricator via cfe-commits]

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

LGTM. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129279

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

[PATCH] D128328: [C++20][Modules] Improve handing of Private Module Fragment diagnostics.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11155
+def err_export_inline_not_defined : Error<
+  "exported inline functions must be defined within the module purview"
+  " and before any private module fragment">;

iains wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > iains wrote:
> > > > iains wrote:
> > > > > ChuanqiXu wrote:
> > > > > > From my reading, 'exported' is not emphasized.
> > > > > it is here:
> > > > > https://eel.is/c++draft/module#private.frag-2.1
> > > > > ( I agree it is somewhat confusing, but the export makes the linkage 
> > > > > external, which the example treats differently from the fn_m() case 
> > > > > which has module linkage).
> > > > > 
> > > > > It is possible that we might need to pull together several pieces of 
> > > > > the std and maybe ask core for clarification?
> > > > > it is here:
> > > > > https://eel.is/c++draft/module#private.frag-2.1
> > > > > ( I agree it is somewhat confusing, but the export makes the linkage 
> > > > > external, which the example treats differently from the fn_m() case 
> > > > > which has module linkage).
> > > > 
> > > > hmm... my linkage comment is wrong - however the distinction between 
> > > > exported and odr-used seems to be made here (fn_m() and fn_e()).
> > > > > 
> > > > > It is possible that we might need to pull together several pieces of 
> > > > > the std and maybe ask core for clarification?
> > > > 
> > > > 
> > > What I read is:
> > > > [dcl.inline]p7: https://eel.is/c++draft/dcl.inline#7
> > > > If an inline function or variable that is attached to a named 
> > > > module is declared in a definition domain, it shall be defined in that 
> > > > domain.
> > > 
> > > and the definition of `definition domain` is:
> > > > [basic.def.odr]p12: https://eel.is/c++draft/basic#def.odr-12
> > > >   A definition domain is a private-module-fragment or the portion 
> > > > of a translation unit excluding its private-module-fragment (if any).
> > > 
> > > The definition of "attached to a named module" is:
> > > > [module.unit]p7: https://eel.is/c++draft/module.unit#7
> > > >  A module is either a named module or the global module. A 
> > > > declaration is attached to a module as follows: ...
> > > 
> > > So it is clearly not consistency with [module.private.frag]p2.1. I would 
> > > send this to WG21.
> > Yes, that was what I found - maybe we are  missing something about the 
> > export that changes those rules.
> > .
> I think that we can consider this closed by the question to the ext reflector 
> and the amendment proposed by core.
I prefer `inline function attached to a named module not defined %select{| 
before the private module fragment}1`. Since the `export` part is not important 
here and the important part is whether or not they are attached to a named 
module.



Comment at: clang/lib/Sema/SemaDecl.cpp:9701
+// have a definition at that point.
+if (isInline && !D.isFunctionDefinition() && getLangOpts().CPlusPlus20 &&
+NewFD->hasOwningModule() &&





Comment at: clang/test/Modules/cxx20-10-5-ex1.cpp:25-28
+export void g(X *x) {
+  fn_s();
+  fn_m();
+}

iains wrote:
> ChuanqiXu wrote:
> > vsapsai wrote:
> > > Can `export inline` function call other non-`export inline` functions? 
> > > Sorry if it is tested somewhere else. Curious what are the transitive 
> > > restrictions, so we test edge cases.
> > I guess it is not tested. But a non-export function wouldn't be exported if 
> > it is called by export function from the perspective of std. Although more 
> > tests should be fine all the time, the current test case should come from 
> > the example in the standard. We could send another test if we want.
> I've changed the example to match the proposed amendment to the standard.
> 
> If you think we should have some other test case (additional to 
> Modules/Reachability-Private), that's fine - would you like to propose one 
> (or maybe add to an existing)?
The current one looks fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128328

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

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126694#3635207 , @iains wrote:

> @rsmith, @ChuanqiXu apologies for the multiple revisions, this has turned out 
> to be much more involved than I imagined from the standard's text.
>
> In D126694#3629254 , @ChuanqiXu 
> wrote:
>
>> In D126694#3629251 , @iains wrote:
>>
>>> In D126694#3629094 , @ChuanqiXu 
>>> wrote:
>>>
 BTW, after I applied the patch, the compiler crashes at 
 https://github.com/ChuanqiXu9/stdmodules.
>>>
>>> That link points to a project - is there (say) a gist of the crash 
>>> information?
>>
>> Here is the crash log:
>
> this code now compiles without error,

Thanks for looking into it!

My personal plan for this revision is to review the details after we add more 
large tests (at least we have a more complete std modules implementation and I 
am trying for it. But I find another bug now). So we might need to wait for a 
while for this patch. How do you think about it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126694

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

[PATCH] D118034: [C++20] [Modules] Don't complain about duplicated default template argument across modules

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 443123.
ChuanqiXu added a comment.

Minor changes.


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

https://reviews.llvm.org/D118034

Files:
  clang/test/Modules/Inputs/redundant-template-default-arg/foo.cppm
  clang/test/Modules/Inputs/redundant-template-default-arg/foo.h
  clang/test/Modules/Inputs/redundant-template-default-arg2/foo.cppm
  clang/test/Modules/Inputs/redundant-template-default-arg3/foo.cppm
  clang/test/Modules/Inputs/redundant-template-default-arg3/foo.h
  clang/test/Modules/Inputs/redundant-template-default-arg3/foo_bad.h
  clang/test/Modules/fwd-declaration.cppm
  clang/test/Modules/redundant-template-default-arg.cpp
  clang/test/Modules/redundant-template-default-arg2.cpp
  clang/test/Modules/redundant-template-default-arg3.cpp

Index: clang/test/Modules/redundant-template-default-arg3.cpp
===
--- clang/test/Modules/redundant-template-default-arg3.cpp
+++ clang/test/Modules/redundant-template-default-arg3.cpp
@@ -1,26 +1,113 @@
 // RUN: rm -rf %t
 // RUN: mkdir %t
-// RUN: %clang_cc1  -std=c++20 %S/Inputs/redundant-template-default-arg3/foo.cppm -I%S/Inputs/redundant-template-default-arg3/. -emit-module-interface -o %t/foo.pcm
-// RUN: %clang_cc1  -fprebuilt-module-path=%t -std=c++20 %s -I%S/Inputs/redundant-template-default-arg3/. -fsyntax-only -verify
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1  -std=c++20 %t/foo.cppm -I%t -emit-module-interface -o %t/foo.pcm
+// RUN: %clang_cc1  -fprebuilt-module-path=%t -std=c++20 %t/use.cpp -I%t/. -fsyntax-only -verify
 
+//--- foo.h
+template 
+T v;
+
+template 
+int v2;
+
+template 
+class my_array {};
+
+template  typename C = my_array>
+int v3;
+
+template 
+T v4;
+
+template 
+T v5;
+
+inline int a = 43;
+template 
+T v6;
+
+inline int b = 43;
+template 
+T v7;
+
+template  2)>
+int v8;
+
+consteval int getInt() {
+  return 55;
+}
+template 
+int v9;
+
+//--- foo_bad.h
+template 
+T v;
+
+template 
+int v2;
+
+template 
+class others_array {};
+
+template  typename C = others_array>
+int v3;
+
+static int a;
+consteval int *getIntPtr() {
+  return &a;
+}
+template 
+T v4;
+
+consteval void *getVoidPtr() {
+  return &a;
+}
+template 
+T v5;
+
+inline int a_ = 43;
+template 
+T v6;
+
+inline int b_ = 43;
+template 
+T v7;
+
+template 
+int v8;
+
+consteval int getInt2() {
+  return 55;
+}
+template 
+int v9;
+
+//--- foo.cppm
+module;
+#include "foo.h"
+export module foo;
+
+//--- use.cpp
 import foo;
 #include "foo_bad.h"
 
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:1 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:1 {{previous default template argument defined in module foo.}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:4 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:4 {{previous default template argument defined in module foo.}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:10 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:10 {{previous default template argument defined in module foo.}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:17 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:13 {{previous default template argument defined in module foo.}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:23 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:16 {{previous default template argument defined in module foo.}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:27 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:20 {{previous default template argument defined in module foo.}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:31 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:24 {{previous default template argument defined in module foo.}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:34 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:27 {{previous default template argument defined in module foo.}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:40 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-defaul

[PATCH] D118034: [C++20] [Modules] Don't complain about duplicated default template argument across modules

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 443124.
ChuanqiXu added a comment.

Minor changes.


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

https://reviews.llvm.org/D118034

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaModule.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Modules/redundant-template-default-arg.cpp
  clang/test/Modules/redundant-template-default-arg2.cpp
  clang/test/Modules/redundant-template-default-arg3.cpp

Index: clang/test/Modules/redundant-template-default-arg3.cpp
===
--- /dev/null
+++ clang/test/Modules/redundant-template-default-arg3.cpp
@@ -0,0 +1,113 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1  -std=c++20 %t/foo.cppm -I%t -emit-module-interface -o %t/foo.pcm
+// RUN: %clang_cc1  -fprebuilt-module-path=%t -std=c++20 %t/use.cpp -I%t/. -fsyntax-only -verify
+
+//--- foo.h
+template 
+T v;
+
+template 
+int v2;
+
+template 
+class my_array {};
+
+template  typename C = my_array>
+int v3;
+
+template 
+T v4;
+
+template 
+T v5;
+
+inline int a = 43;
+template 
+T v6;
+
+inline int b = 43;
+template 
+T v7;
+
+template  2)>
+int v8;
+
+consteval int getInt() {
+  return 55;
+}
+template 
+int v9;
+
+//--- foo_bad.h
+template 
+T v;
+
+template 
+int v2;
+
+template 
+class others_array {};
+
+template  typename C = others_array>
+int v3;
+
+static int a;
+consteval int *getIntPtr() {
+  return &a;
+}
+template 
+T v4;
+
+consteval void *getVoidPtr() {
+  return &a;
+}
+template 
+T v5;
+
+inline int a_ = 43;
+template 
+T v6;
+
+inline int b_ = 43;
+template 
+T v7;
+
+template 
+int v8;
+
+consteval int getInt2() {
+  return 55;
+}
+template 
+int v9;
+
+//--- foo.cppm
+module;
+#include "foo.h"
+export module foo;
+
+//--- use.cpp
+import foo;
+#include "foo_bad.h"
+
+// expected-error@foo_bad.h:1 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:1 {{previous default template argument defined in module foo.}}
+// expected-error@foo_bad.h:4 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:4 {{previous default template argument defined in module foo.}}
+// expected-error@foo_bad.h:10 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:10 {{previous default template argument defined in module foo.}}
+// expected-error@foo_bad.h:17 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:13 {{previous default template argument defined in module foo.}}
+// expected-error@foo_bad.h:23 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:16 {{previous default template argument defined in module foo.}}
+// expected-error@foo_bad.h:27 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:20 {{previous default template argument defined in module foo.}}
+// expected-error@foo_bad.h:31 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:24 {{previous default template argument defined in module foo.}}
+// expected-error@foo_bad.h:34 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:27 {{previous default template argument defined in module foo.}}
+// expected-error@foo_bad.h:40 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:33 {{previous default template argument defined in module foo.}}
Index: clang/test/Modules/redundant-template-default-arg2.cpp
===
--- /dev/null
+++ clang/test/Modules/redundant-template-default-arg2.cpp
@@ -0,0 +1,38 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/foo.cppm -I%t -emit-module-interface -o %t/foo.pcm
+// RUN: %clang_cc1 -fprebuilt-module-path=%t -std=c++20 %t/use.cpp -fsyntax-only -verify
+
+//--- foo.cppm
+export module foo;
+export template 
+T v;
+
+export template 
+int v2;
+
+export template 
+class my_array {};
+
+export template  typename C = my_array>
+int v3;
+
+//--- use.cpp
+import foo;
+template 
+T v; // expected-error {{declaration of 'v' in the global module follows declaration in module foo}}
+ // expected-n...@foo.cppm:3 {{previous declaration is here}}
+
+template 
+int v2; // expected-error {{declaration of 'v2' in the global module follows declaration in module foo}}
+// expected-n...@foo.cppm:6 {{previous declaration is here}}
+
+template 
+class my_array {}; // expected-error {{redefinition of 'my_array'}}
+   // expected-n...@foo.cppm:9 {{previous definition is here}}
+
+template  typename C = my_array>
+int v3; // expected-

[PATCH] D118034: [C++20] [Modules] Don't complain about duplicated default template argument across modules

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu closed this revision.
ChuanqiXu added a comment.

Committed in https://reviews.llvm.org/rG354a597b9f3aad2a6a37518d47351adb99688cd2


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

https://reviews.llvm.org/D118034

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

[PATCH] D127187: [C++20] [Modules] Implement AllAdditionalTUReachable

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

The direction is approved in https://reviews.llvm.org/D113545. @MaskRay might 
you help to review the style?


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

https://reviews.llvm.org/D127187

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