This is an archive of the discontinued LLVM Phabricator instance.

Differential D115778

[docs] Give the reason why the support for coroutine is partial
ClosedPublic

Authored by ChuanqiXu on Dec 14 2021, 6:23 PM.

Details

Reviewers
aaron.ballman
jyknight
Commits
rG5653d127d7c4: [docs] Give the reason why the support for coroutine is partial
Summary

See https://lists.llvm.org/pipermail/cfe-dev/2021-December/069608.html. We couldn't mark coroutine as done now due to there to critical bugs. And as Aaron suggests, it would be better to give the reason in the docs.

Diff Detail

Unit TestsFailed

TimeTest
1,600 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::non-executable-pc.cpp
60 msx64 debian > LLVM.Bindings/Go::go.test

Event Timeline

ChuanqiXu requested review of this revision.Dec 14 2021, 6:23 PM
ChuanqiXu created this revision.
Herald added a subscriber: cfe-commits. · View Herald TranscriptDec 14 2021, 6:23 PM
Harbormaster completed remote builds in B139352: Diff 394439.Dec 14 2021, 7:01 PM
aaron.ballman added a comment.Dec 15 2021, 4:28 AM

Thanks for this!

clang/www/cxx_status.html
1202

Rather than following this approach, it'd be nice to have the information provided directly inline, as done in: https://github.com/llvm/llvm-project/blob/main/clang/www/cxx_status.html#L916

1267–1270

Also, when moving it to be a detail of the current row, I'd drop the (12): from the text.

I think we should also add some words to the effect of "This feature was implemented based on the Coroutines TS and requires further analysis to determine what support remains to be added." This should hopefully make it clear that the list of issues found is not exhaustive and that investigative work is still needed.

ChuanqiXu updated this revision to Diff 394527.Dec 15 2021, 4:38 AM

Address comments.

ChuanqiXu added inline comments.Dec 15 2021, 4:41 AM
clang/www/cxx_status.html
1267–1270

The current implementation have some minor differences with coroutines TS already. So I chose the wording Richard gives in the mailing list. I think it is proper.

aaron.ballman accepted this revision.Dec 15 2021, 4:45 AM

LGTM aside from a minor wording fix. Thank you for this!

clang/www/cxx_status.html
1206–1207

Fixes typo and grammar but hopefully gets the same point across.

This revision is now accepted and ready to land.Dec 15 2021, 4:45 AM
ChuanqiXu updated this revision to Diff 394529.Dec 15 2021, 4:56 AM

Address comments. Thanks for reviewing.

This revision was landed with ongoing or failed builds.Dec 15 2021, 5:02 AM
Closed by commit rG5653d127d7c4: [docs] Give the reason why the support for coroutine is partial (authored by ChuanqiXu). · Explain Why
This revision was automatically updated to reflect the committed changes.
ChuanqiXu added a commit: rG5653d127d7c4: [docs] Give the reason why the support for coroutine is partial.
Harbormaster completed remote builds in B139414: Diff 394529.Dec 15 2021, 5:30 AM

Revision Contents

PathSize
clang/
www/
8 lines

Diff 394439

clang/www/cxx_status.html

Show First 20 Lines • Show All 1,193 Lines • ▼ Show 20 Lines </tr>
</tr> </tr>
<tr> <tr>
<td><a href="https://wg21.link/p1815r2">P1815R2</a></td> <td><a href="https://wg21.link/p1815r2">P1815R2</a></td>
<td class="none" align="center">No</td> <td class="none" align="center">No</td>
</tr> </tr>
<tr> <tr>
<td>Coroutines</td> <td>Coroutines</td>
<td><a href="https://wg21.link/p0912r5">P0912R5</a></td> <td><a href="https://wg21.link/p0912r5">P0912R5</a></td>
<td class="partial" align="center">Partial</td> <td class="partial" align="center">Partial <a href="#CoroPartialReason">(12)</a></td>
aaron.ballmanUnsubmitted
Not Done
ReplyInline Actions

Rather than following this approach, it'd be nice to have the information provided directly inline, as done in: https://github.com/llvm/llvm-project/blob/main/clang/www/cxx_status.html#L916

aaron.ballman: Rather than following this approach, it'd be nice to have the information provided directly…
</tr> </tr>
<!-- Cologne 2019 papers --> <!-- Cologne 2019 papers -->
<tr> <tr>
<td>Deprecate <tt>a[b,c]</tt></td> <td>Deprecate <tt>a[b,c]</tt></td>
<td><a href="https://wg21.link/p1161r3">P1161R3</a></td> <td><a href="https://wg21.link/p1161r3">P1161R3</a></td>
aaron.ballmanUnsubmitted
Not Done
ReplyInline Actions
`__attribute__((const))` attribute correctly. There can be issues where the
- coroutine may resume on a different thread. Also there are works reamined to
- check through the sections on coroutines in the C++ standard.
+ coroutine may resume on a different thread.<br \>This feature requires further
+ analysis of the C++ Standard to determine what work is necessary for conformance.
</details></td>

Fixes typo and grammar but hopefully gets the same point across.

aaron.ballman: Fixes typo and grammar but hopefully gets the same point across.
<td class="full" align="center">Clang 9</td> <td class="full" align="center">Clang 9</td>
</tr> </tr>
<tr> <tr>
<td>Deprecate some problematic uses of <tt>volatile</tt></td> <td>Deprecate some problematic uses of <tt>volatile</tt></td>
<td><a href="https://wg21.link/p1152r4">P1152R4</a></td> <td><a href="https://wg21.link/p1152r4">P1152R4</a></td>
<td class="full" align="center">Clang 10</td> <td class="full" align="center">Clang 10</td>
</tr> </tr>
<tr> <tr>
▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Lines </tr>
</tr> </tr>
</table> </table>
<p> <p>
<span id="p0482">(11): Prior to Clang 8, this feature is not enabled by <span id="p0482">(11): Prior to Clang 8, this feature is not enabled by
<tt>-std=c++20</tt>, but can be enabled with <tt>-fchar8_t</tt>. <tt>-std=c++20</tt>, but can be enabled with <tt>-fchar8_t</tt>.
</span> </span>
</p> </p>
<p>
<span id="CoroPartialReason">(12): The optimizer couldn't handle TLS with
`__attribute__((const))` attribute correctly. It implies that there would
be problems if the coroutine is possible to resume on a different thread.
</span>
aaron.ballmanUnsubmitted
Not Done
ReplyInline Actions
</p>
<p>
- <span id="CoroPartialReason">(12): The optimizer couldn't handle TLS with
- `__attribute__((const))` attribute correctly. It implies that there would
- be problems if the coroutine is possible to resume on a different thread.
+ <span id="CoroPartialReason">(12): The optimizer does not yet handle TLS with
+ `__attribute__((const))` attribute correctly. There can be issues where the
+ coroutine may resume on a different thread.
</span>
</p>

Also, when moving it to be a detail of the current row, I'd drop the (12): from the text.

I think we should also add some words to the effect of "This feature was implemented based on the Coroutines TS and requires further analysis to determine what support remains to be added." This should hopefully make it clear that the list of issues found is not exhaustive and that investigative work is still needed.

aaron.ballman: Also, when moving it to be a detail of the current row, I'd drop the `(12): ` from the text. I…
ChuanqiXuAuthorUnsubmitted
Done
ReplyInline Actions

The current implementation have some minor differences with coroutines TS already. So I chose the wording Richard gives in the mailing list. I think it is proper.

ChuanqiXu: The current implementation have some minor differences with coroutines TS already. So I chose…
</p>
</details> </details>
<h2 id="cxx23">C++2b implementation status</h2> <h2 id="cxx23">C++2b implementation status</h2>
<p>Clang has support for some of the features of the C++ standard following <p>Clang has support for some of the features of the C++ standard following
C++20, informally referred to as C++2b.</p> C++20, informally referred to as C++2b.</p>
<p>You can use Clang in C++2b mode with the <code>-std=c++2b</code> option.</p> <p>You can use Clang in C++2b mode with the <code>-std=c++2b</code> option.</p>
▲ Show 20 Lines • Show All 239 Lines • Show Last 20 Lines