Page MenuHomePhabricator

[libcxx] [coroutine] Conform coroutine implementation
ClosedPublic

Authored by ChuanqiXu on Sep 8 2021, 4:40 AM.

Details

Summary

This revision is based on @Quuxplusone's suggestion in D108697.
This patch aims to conform the coroutine implementation to C++20 standard.

The reference for this patch is: https://eel.is/c++draft/support.coroutine

The main change of this patch includes:

  • Require _LIBCPP_STD_VER > 11. This keeps consistency with tests and gcc.
  • Replace _LIBCPP_CONSTEXPR and _NOEXCEPT with constexpr and noexcept.
  • Add missing const and noexcept qualifier for functions.
  • Break the inheritance relationship of coroutine_handle<Promise>, coroutine_handle<void> and noop_coroutine_handle based on the specification.
  • Remove redundant overloads for from_address.
  • Use operator<=> to replace <, >, =>, <=.
  • Use stable names throughout like` [coroutine.handle.compare] to replace something like 18.11.2.7 comparison operators`.
  • Use _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY.

Test Plan: check-all

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ChuanqiXu added inline comments.Oct 24 2021, 7:12 PM
libcxx/include/__coroutine/coroutine_handle.h
88

Reply to lewis's comment:

We could implement `__is_suspended` by adding another bits in the coroutine frame and pay for the cost for an extra store instruction before every suspend. 

To my mind, the intention of this patch is conforming the implementation of libcxx/coroutine to the standard. Since the `__is_suspended` API is not a part of the standard. I think we could ignore `__is_suspended` now. IMO, if we want to support `__is_suspended`, we need to submit another proposal to LWG. Otherwise, we shouldn't implement it directly or we should delete it simply.
ChuanqiXu added a project: Restricted Project.Oct 25 2021, 1:40 AM
ChuanqiXu updated this revision to Diff 382172.Oct 25 2021, 8:25 PM

Run generate_private_header_tests.py and adding the inclusion check for <experimental/coroutine>.

Fix test ''libcxx/utils/ci/run-buildbot check-generated-output". I also checked generic-cxx2b and generic-cxx20 locally. Hope it could pass CI this time.
Noticed that "libcxx/utils/ci/run-buildbot check-format" would still fail. But I remembered that we shouldn't apply clang-check on libcxx. @Quuxplusone so could we ignore the failed test from libcxx/utils/ci/run-buildbot check-format?

Fix test ''libcxx/utils/ci/run-buildbot check-generated-output". I also checked generic-cxx2b and generic-cxx20 locally. Hope it could pass CI this time.
Noticed that "libcxx/utils/ci/run-buildbot check-format" would still fail. But I remembered that we shouldn't apply clang-check on libcxx. @Quuxplusone so could we ignore the failed test from libcxx/utils/ci/run-buildbot check-format?

Right. At the moment CI says "libcxx CI (Format, C++2b, ...) failed". You must fix the "C++2b, ..." parts, but you can leave the "(Format)" part failing. It's sort of like warnings and errors: As long as the CI is red (because of "C++2b, ..." failures), it will also mention that "Format" failed. But, if the only thing that is failing is "Format", then the CI will be green. "Format" is just a warning, not an error.

Fix test ''libcxx/utils/ci/run-buildbot check-generated-output". I also checked generic-cxx2b and generic-cxx20 locally. Hope it could pass CI this time.
Noticed that "libcxx/utils/ci/run-buildbot check-format" would still fail. But I remembered that we shouldn't apply clang-check on libcxx. @Quuxplusone so could we ignore the failed test from libcxx/utils/ci/run-buildbot check-format?

Right. At the moment CI says "libcxx CI (Format, C++2b, ...) failed". You must fix the "C++2b, ..." parts, but you can leave the "(Format)" part failing. It's sort of like warnings and errors: As long as the CI is red (because of "C++2b, ..." failures), it will also mention that "Format" failed. But, if the only thing that is failing is "Format", then the CI will be green. "Format" is just a warning, not an error.

From what I see in the CI system, the reason why C++2b and C++2a failed is that the compiler wouldn't search in std:: namespace. In another word, the CI system would become green after D108696 checked in.

From what I see in the CI system, the reason why C++2b and C++2a failed is that the compiler wouldn't search in std:: namespace. In another word, the CI system would become green after D108696 checked in.

D108696 is already correctly listed as a "Parent Revision" of D109433, so D109433's CI is already running on the-combination-of-both-of-them. And it appears that it's still failing. I believe part of the reason is identified in my comments above. I recommend fixing that, also fixing anything that needs fixing in D108696, then doing "Update Diff" on D108696, waiting an hour, doing "Update Diff" on D109433, and then waiting for the CI to complete on both. If that continues to fail CI, then that continues to be a problem (and I would by default assume it's a problem with the patches, not a problem with the CI setup).

libcxx/include/__config
1275–1276

I believe this diff is the reason your tests are failing.
Based on a quick skim, I think the deal is:

  • __cpp_coroutines is set when the compiler supports <experimental/coroutine>
  • __cpp_impl_coroutine is set when the compiler supports <coroutine>

So we might want something here like

#if !(defined(__cpp_coroutines) && __cpp_coroutines >= 201703L)
#define _LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES
#endif

#if !(defined(__cpp_impl_coroutine) && __cpp_impl_coroutine >= 201902L)
#define _LIBCPP_HAS_NO_CXX20_COROUTINES
#endif

and then check the appropriate macro in each coroutine header.

This will also need to be reflected in generate_header_tests.py.

libcxx/include/__coroutine/coroutine_handle.h
11

Please rename the include guard to _LIBCPP___COROUTINE_COROUTINE_HANDLE_H.

libcxx/include/__coroutine/coroutine_noop.h
10 ↗(On Diff #382207)

Please rename the file to __coroutine/noop_coroutine_handle.h, and the include guard to _LIBCPP___COROUTINE_NOOP_COROUTINE_HANDLE_H.

libcxx/include/__coroutine/coroutine_traits.h
11

Please rename the include guard to _LIBCPP___COROUTINE_COROUTINE_TRAITS_H.

ChuanqiXu added a project: Restricted Project.Oct 29 2021, 2:17 AM
ChuanqiXu updated this revision to Diff 383283.Oct 29 2021, 2:53 AM
ChuanqiXu removed a project: Restricted Project.
  • Rename sub header name and macro as suggested.
  • Move the change part in clang into previous revision D108696.

I suspect the reason why it is failing is that the original version contains the change in clang too! So the CI didn't compile it. Let's see if it could work.

ChuanqiXu marked 2 inline comments as done.Oct 29 2021, 2:57 AM

From what I see in the CI system, the reason why C++2b and C++2a failed is that the compiler wouldn't search in std:: namespace. In another word, the CI system would become green after D108696 checked in.

D108696 is already correctly listed as a "Parent Revision" of D109433, so D109433's CI is already running on the-combination-of-both-of-them. And it appears that it's still failing. I believe part of the reason is identified in my comments above. I recommend fixing that, also fixing anything that needs fixing in D108696, then doing "Update Diff" on D108696, waiting an hour, doing "Update Diff" on D109433, and then waiting for the CI to complete on both. If that continues to fail CI, then that continues to be a problem (and I would by default assume it's a problem with the patches, not a problem with the CI setup).

I guess the reason is that this patch included the change in clang part and it doesn't tag clang. So the change isn't compiled in CI. Now I move the change in clang part into the parent revision. Let's see if it could works : )

libcxx/include/__config
1275–1276

_LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES is defined in experimental/__config now. I think the place is better.
And I remain the name _LIBCPP_HAS_NO_COROUTINES instead of _LIBCPP_HAS_NO_CXX20_COROUTINES since it looks a little bit redundancy.

ChuanqiXu updated this revision to Diff 383336.Oct 29 2021, 6:51 AM

Update generated test

ChuanqiXu added a project: Restricted Project.Oct 29 2021, 7:16 AM
Quuxplusone added inline comments.Oct 29 2021, 8:24 AM
libcxx/include/__config
1275–1276

FWIW, I'm okay with "keeping" the name _LIBCPP_HAS_NO_COROUTINES; but I do still think it's better to change all uses of _LIBCPP_HAS_NO_COROUTINES to _LIBCPP_HAS_NO_{CXX20,EXPERIMENTAL}_COROUTINES. Both for programming reasons (so that the future reader can unambiguously see that a certain codepath is related to C++20 or experimental or both, based on the name(s) of the macros involved), and for engineering reasons (so that the patch-writer is forced to touch every instance of _LIBCPP_HAS_NO_COROUTINES in the entire codebase, and any _LIBCPP_HAS_NO_COROUTINES that remains in the codebase can be instantly identified as an oversight, not just something that coincidentally didn't need to be changed).

Anyway, I assume you do intend that one of these macros relates to experimental coroutines and the other one relates specifically to the non-experimental C++20 coroutines. Right?:

#if defined(_LIBCPP_HAS_NO_{CXX20_,}COROUTINES) && defined(_LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES)
// we have no coroutine support at all
#else
// we have _some_ kind of coroutines (maybe C++20, maybe experimental)
#endif

@Quuxplusone for the still failing CI system, I believe it didn't combine D108696. Here is my reason:

So it didn't contain the codes in D108696. So if we care the CI result, we could wait after D108696 merged in.

libcxx/include/__config
1275–1276

Yeah, I am not a defender on the names. Since I am not a native speaker, so I trust your sensitivity on names should be right. I would rename it later.

ChuanqiXu updated this revision to Diff 384657.Nov 3 2021, 9:57 PM

Rename _LIBCPP_HAS_NO_COROUTINES to _LIBCPP_HAS_NO_CXX20_COROUTINES.

Oh, it is weird that the CI keeps failing after landing D108696. I have tested many many times locally. I am wondering if the compiler used in the CI system didn't get updated in time. Let's see if it could be green tomorrow.

I had an experiments in https://reviews.llvm.org/D113170. The diff there is the same with here except that diff would check if the compiler introduces __cpp_impl_coroutine which should be introduced in previous commit: https://github.com/llvm/llvm-project/commit/ec117158a390a0ebf64377caa5abd0c976df8f7a. And the results there shows that the compiler in the CI system didn't contain macro __cpp_impl_coroutine so that's the reason for failing.
I guess it would be updated in days. Let's wait some days.

Quuxplusone requested changes to this revision.Fri, Nov 5, 8:52 AM
Quuxplusone added inline comments.
libcxx/test/std/language.support/support.coroutines/coroutine.handle/void_handle.pass.cpp
10

libc++ needs to work on Clang 13.x as well as trunk/14.x. Clang 13.x doesn't define __cpp_impl_coroutine. Therefore, to fix the buildkite, I think you need to introduce a new feature flag so that you can say UNSUPPORTED: libcpp-no-coroutines on all of these new tests. Grep for libcpp-no-concepts and see if the process is self-explanatory... or ask @ldionne what's the right way to do it. :)

This revision now requires changes to proceed.Fri, Nov 5, 8:52 AM
ChuanqiXu updated this revision to Diff 385226.Fri, Nov 5, 8:32 PM

Disable newly added tests for 13.x by adding libcpp-no-coroutines feature. I have tested 13.x locally. Hope CI could get green this time : (

ChuanqiXu updated this revision to Diff 385234.Fri, Nov 5, 10:22 PM

re-check check-generated-output

ChuanqiXu updated this revision to Diff 385244.Sat, Nov 6, 1:00 AM
  • Include <version> in coroutine
  • Export compare for coroutine in module map.

I'm not sure I'm reading the test results right, but if I am, then it looks like we're still skipping all your new tests — the test runner thinks that Clang 14.x doesn't support __cpp_impl_coroutine.
https://buildkite.com/llvm-project/libcxx-ci/builds/6457
But I have verified locally that Clang trunk (14.x) clang++ -std=c++20 -dM -E x.cpp does output #define __cpp_impl_coroutine 201902L, so I think this is just due to buildkite using an old build... I'm not sure. We still shouldn't land this until we can prove that buildkite will run (and pass) these new tests.

libcxx/utils/generate_header_tests.py
58–60

I think we can now change these lines to

"experimental/coroutine": ["ifndef _LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES"],
"coroutine": ["ifndef _LIBCPP_HAS_NO_CXX20_COROUTINES"],

for better consistency with all the other features in this list.

The build system uses https://apt.llvm.org/ for its packages. This isn't updated nightly so it might be the build used an old version. The current timestamp of the build is 20211106103030. @ChuanqiXu can you rebase this patch to trigger the CI again?

ChuanqiXu updated this revision to Diff 385386.Sun, Nov 7, 6:10 PM

Address comments.

ChuanqiXu marked an inline comment as done.Sun, Nov 7, 9:44 PM

The build system uses https://apt.llvm.org/ for its packages. This isn't updated nightly so it might be the build used an old version. The current timestamp of the build is 20211106103030. @ChuanqiXu can you rebase this patch to trigger the CI again?

@Mordante Due to my system is not Debian/Ubuntu, I couldn't test the package in the link locally. And I sent another diff in: https://reviews.llvm.org/D113170. It is the same with this one except that it would check the macro 'cpp_impl_coroutine'. And the result shows that the compiler in the CI still didn't emit macro 'cpp_impl_coroutine'. Could you help to check the compiler in the CI system or tell me how to test the timestamp of the compiler in the CI system? Thanks.

The build system uses https://apt.llvm.org/ for its packages. This isn't updated nightly so it might be the build used an old version. The current timestamp of the build is 20211106103030. @ChuanqiXu can you rebase this patch to trigger the CI again?

@Mordante Due to my system is not Debian/Ubuntu, I couldn't test the package in the link locally. And I sent another diff in: https://reviews.llvm.org/D113170. It is the same with this one except that it would check the macro 'cpp_impl_coroutine'. And the result shows that the compiler in the CI still didn't emit macro 'cpp_impl_coroutine'. Could you help to check the compiler in the CI system or tell me how to test the timestamp of the compiler in the CI system? Thanks.

I run Debian so I tested locally which version was available in apt.llvm.org. I just did a quick test with the Docker image we run on our CI. There the version installed is of the 27th of October. When I manually update in Docker I get a binary of the 5th of November. (Not sure why Ubuntu has a one day older build than Debian). So it seems our CI uses a version of Clang before D108696 landed. I have some suspicions, but @ldionne knows more about our CI.

General comment regarding stdlib/compiler compatibility.

I notice that you are keeping the std::experimental::coroutine_handle types but leaving them as separate types rather than making them type-aliases of the std::coroutine_handle types.

As it has been explained by @ChuanqiXu , the plan is to first add support for C++20 coroutines while keeping around experimental coroutines for two releases. After two releases, we remove the experimental ones and any user that hasn't migrated yet will be broken. This is the policy we have for experimental features in libc++, which tries to balance keeping the library sane without being too eager to break users.

Is the intention to just require everyone to move to a clang compiler that supports std::coroutine_handle and abandon support for std::experimental types?

Yes, but after two releases. During this two-release time frame, both experimental and non-experimental coroutines should keep working. We probably don't want to allow mixing the two for sanity, though. My understanding is that this is what D108696 achieves.

Once the clang compiler has been updated to reference the new types, is there any point in continuing to keep the std::experimental types around?

Just to avoid breaking users too eagerly. We want to give them a bit of time to migrate, but then we should nuke std::experimental as soon as we can to avoid user confusion and reduce our maintenance costs.

I run Debian so I tested locally which version was available in apt.llvm.org. I just did a quick test with the Docker image we run on our CI. There the version installed is of the 27th of October. When I manually update in Docker I get a binary of the 5th of November. (Not sure why Ubuntu has a one day older build than Debian). So it seems our CI uses a version of Clang before D108696 landed. I have some suspicions, but @ldionne knows more about our CI.

Long story short, our CI doesn't get an updated version of the compiler on apt.llvm.org automatically. They cache the Docker image and don't rebuild it unless the Dockerfile changes AFAICT. This isn't great -- we should always use the Dockerfile in the repo to run the build, but there are hurdles to setting that up. I'll manually change the Dockerfile to trigger an image rebuild -- we should pick up the latest apt.llvm.org compiler, which I think should include D108696.

By the way, thanks a lot to everyone for chiming in and reviewing this. I've been MIA because I'm swamped by other work, but please keep pinging me on Discord for specific questions.

ChuanqiXu updated this revision to Diff 385685.Mon, Nov 8, 7:25 PM

Rebase with trunk.

Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Nov 8, 7:25 PM

The build system uses https://apt.llvm.org/ for its packages. This isn't updated nightly so it might be the build used an old version. The current timestamp of the build is 20211106103030. @ChuanqiXu can you rebase this patch to trigger the CI again?

@Mordante Due to my system is not Debian/Ubuntu, I couldn't test the package in the link locally. And I sent another diff in: https://reviews.llvm.org/D113170. It is the same with this one except that it would check the macro 'cpp_impl_coroutine'. And the result shows that the compiler in the CI still didn't emit macro 'cpp_impl_coroutine'. Could you help to check the compiler in the CI system or tell me how to test the timestamp of the compiler in the CI system? Thanks.

I run Debian so I tested locally which version was available in apt.llvm.org. I just did a quick test with the Docker image we run on our CI. There the version installed is of the 27th of October. When I manually update in Docker I get a binary of the 5th of November. (Not sure why Ubuntu has a one day older build than Debian). So it seems our CI uses a version of Clang before D108696 landed. I have some suspicions, but @ldionne knows more about our CI.

Got it. Thanks.

I run Debian so I tested locally which version was available in apt.llvm.org. I just did a quick test with the Docker image we run on our CI. There the version installed is of the 27th of October. When I manually update in Docker I get a binary of the 5th of November. (Not sure why Ubuntu has a one day older build than Debian). So it seems our CI uses a version of Clang before D108696 landed. I have some suspicions, but @ldionne knows more about our CI.

Long story short, our CI doesn't get an updated version of the compiler on apt.llvm.org automatically. They cache the Docker image and don't rebuild it unless the Dockerfile changes AFAICT. This isn't great -- we should always use the Dockerfile in the repo to run the build, but there are hurdles to setting that up. I'll manually change the Dockerfile to trigger an image rebuild -- we should pick up the latest apt.llvm.org compiler, which I think should include D108696.

It looks like that the CI didn't update the compiler yet. May I ask when would it be OK to use a later version?

ChuanqiXu updated this revision to Diff 386095.Wed, Nov 10, 1:50 AM

Now it shows that the compiler should be updated. And there are two tests failing:

  • libcxx/utils/ci/run-buildbot generic-modules
  • libcxx/utils/ci/run-buildbot generic-32bit
    • libcxx/gdb/gdb_pretty_printer_test.sh.cpp

I don't find the reason why generic-modules fails now. And it is weird that libcxx/gdb/gdb_pretty_printer_test.sh.cpp is failing since it is irrelevant with coroutine. I would test them locally.

ChuanqiXu updated this revision to Diff 386120.Wed, Nov 10, 4:19 AM

Fix the test failure for generic modules.
I finally found the reason why it would fail for generic modules. Since the tests didn't contain headers which is included in the __coroutine/* headers even if they are already included! It is surprising to people who didn't use clang module before. Then the only failure is for libcxx/gdb/gdb_pretty_printer_test.sh.cpp. It looks not relevant with coroutines. But I would try to look into it in case it would fail still.

When I look into the error message, I find the following error message:

FAIL: /home/libcxx-builder/.buildkite-agent/builds/344e6285b245-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:247
GDB printed:
   'std::bitset<258u>'
Value should match:
   'std::bitset<258(ul)?>'
FAIL: /home/libcxx-builder/.buildkite-agent/builds/344e6285b245-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:250
GDB printed:
   'std::bitset<0u>'
Value should match:
   'std::bitset<0(ul)?>'
FAIL: /home/libcxx-builder/.buildkite-agent/builds/344e6285b245-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:253
GDB printed:
   'std::bitset<15u> = {[2] = 1, [3] = 1, [4] = 1, [5] = 1, [6] = 1, [7] = 1, [8] = 1, [9] = 1}'
Value should match:
   'std::bitset<15(ul)?> = {\\[2\\] = 1, \\[3\\] = 1, \\[4\\] = 1, \\[5\\] = 1, \\[6\\] = 1, \\[7\\] = 1, \\[8\\] = 1, \\[9\\] = 1}'
FAIL: /home/libcxx-builder/.buildkite-agent/builds/344e6285b245-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:261
GDB printed:
   'std::bitset<258u> = {[0] = 1, [129] = 1, [132] = 1}'
Value should match:
   'std::bitset<258(ul)?> = {\\[0\\] = 1, \\[129\\] = 1, \\[132\\] = 1}'

It looks like this is introduced in https://reviews.llvm.org/D113112. @ldionne hi, could you help to look into this? @Quuxplusone Now I think this test failure should be irrelevant with this diff and all other tests are passed. Do you think we should land this after the bug fixed or we could land this now?

[gdb_pretty_printer_test.sh.cpp is failing]

It looks like this is introduced in https://reviews.llvm.org/D113112. @ldionne hi, could you help to look into this? @Quuxplusone Now I think this test failure should be irrelevant with this diff and all other tests are passed. Do you think we should land this after the bug fixed or we could land this now?

  • I'm 100% sure that the GDB-pretty-printer test failure is not your fault.
  • I think @ldionne should look into it (in fact, my impression is that he already is). Maybe the solution will just be to UNSUPPORTED it temporarily, but let's actually get it green.
  • IMO this coroutines stuff is big and important enough that you should wait until that pretty-printer test has been fixed, and get a really 100% green CI run, before you land it. It's been a few months; waiting another few days won't hurt anything. And green CI runs for big features are nice to have. So I don't think you should land it until CI is green.

[gdb_pretty_printer_test.sh.cpp is failing]

It looks like this is introduced in https://reviews.llvm.org/D113112. @ldionne hi, could you help to look into this? @Quuxplusone Now I think this test failure should be irrelevant with this diff and all other tests are passed. Do you think we should land this after the bug fixed or we could land this now?

  • I'm 100% sure that the GDB-pretty-printer test failure is not your fault.
  • I think @ldionne should look into it (in fact, my impression is that he already is). Maybe the solution will just be to UNSUPPORTED it temporarily, but let's actually get it green.
  • IMO this coroutines stuff is big and important enough that you should wait until that pretty-printer test has been fixed, and get a really 100% green CI run, before you land it. It's been a few months; waiting another few days won't hurt anything. And green CI runs for big features are nice to have. So I don't think you should land it until CI is green.

Got it. Thanks for the explanation!

@Quuxplusone the CI is finally green now!

ldionne accepted this revision.Mon, Nov 15, 2:16 PM

LGTM with some suggestions. I am assuming that you copy-pasted the code and tests from experimental/coroutine and performed minor modifications as explained in the description.

Thanks!

libcxx/include/CMakeLists.txt
134–137

Please sort!

libcxx/include/__coroutine/coroutine_handle.h
98

I *think* this block can be removed entirely, but let's not block this on D113938.

libcxx/include/__coroutine/noop_coroutine_handle.h
24

Can we get rid of this (and similarly in the corresponding test file)?

Quuxplusone added inline comments.Mon, Nov 15, 2:53 PM
libcxx/include/CMakeLists.txt
134–137

And before sorting, please rename coroutine_trivial_awaitables to just trivial_awaitables (and update the modulemap, and the header guard, and the generated tests). I just noticed this. :P

ChuanqiXu updated this revision to Diff 387467.Mon, Nov 15, 6:21 PM

Rename coroutine_trivial_awaitables to trivial_awaitables and sort CMakeLists, modulemap .
(Didn't change the header guard since I found it is consistency now)

ChuanqiXu marked 2 inline comments as done.Mon, Nov 15, 6:24 PM
ChuanqiXu added inline comments.
libcxx/include/__coroutine/noop_coroutine_handle.h
24

Only Clang and MSVC implements __builtin_coro_noop. GCC uses other tricks. So if we move get rid of this guard, it couldn't compiled by GCC. According to our previous discussion, we decide to support this in later patches.

ChuanqiXu updated this revision to Diff 387481.Mon, Nov 15, 7:36 PM

Rename correctly.

This revision was not accepted when it landed; it landed in state Needs Review.Mon, Nov 15, 10:15 PM
This revision was automatically updated to reflect the committed changes.

@Quuxplusone @ldionne many thanks for your helps! I couldn't make this without your helps.