Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-154: Lazy & RescheduleLazy migrate to LazyBase #155

Merged
merged 7 commits into from
Sep 19, 2022

Conversation

yanminhui
Copy link
Collaborator

Why

Closes #154

Lazy and RescheduleLazy migrate to LazyBase.

What is changing

Example

@CLAassistant
Copy link

CLAassistant commented Sep 17, 2022

CLA assistant check
All committers have signed the CLA.

@yanminhui
Copy link
Collaborator Author

FIXME: testLazyPerf caused by #156 .

async_simple/coro/Lazy.h Outdated Show resolved Hide resolved
Comment on lines +301 to +304
explicit LazyBase(Handle coro) : _coro(coro) {}
LazyBase(LazyBase&& other) : _coro(std::move(other._coro)) {
other._coro = nullptr;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

构造函数做成 protected 的会比较好,LazyBase 不应该可以被直接构造

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LazyBase 定义在命名空间 detail 内部,用户不应使用。

若把构造函数放在 protected 里面,由于 protected 构造函数无法被继承,将导致派生类全部要重新声明一遍,再委派给 LazyBase

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm,detail namespace 算弱约束吧,不过我不算很纠结这个

async_simple/coro/Lazy.h Outdated Show resolved Hide resolved
async_simple/coro/Lazy.h Outdated Show resolved Hide resolved
async_simple/coro/Lazy.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@RainMark RainMark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest,lgtm

@ChuanqiXu9
Copy link
Collaborator

另外你需要签署 CLA 协议我们才能合入你的代码

Copy link
Collaborator

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@ChuanqiXu9 ChuanqiXu9 merged commit 53361ef into alibaba:main Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants