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

[IR Operation] ⚔Elden chapter 1.3⚔ #55657

Merged
merged 9 commits into from
Aug 1, 2023
Merged

Conversation

gouzil
Copy link
Member

@gouzil gouzil commented Jul 24, 2023

PR types

Others

PR changes

Others

Description

#55205 case 1.3

@@ -214,6 +214,10 @@ Region *Operation::GetParentRegion() const {
return parent_ ? parent_->GetParent() : nullptr;
}

Region *Operation::GetParentRegion() {
Copy link
Member Author

Choose a reason for hiding this comment

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

需要研发大哥确认一下这个要不要加

Copy link
Contributor

Choose a reason for hiding this comment

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

这个我们内部之前讨论了下,其实这里的接口按照规范要分为两个:

const X* GetX() const;

X* GetX();

即 const 对象接口应该返回const对象,非const接口返回非const对象。
但似乎这里存量代码本身也存在类似的问题。

Copy link
Contributor

Choose a reason for hiding this comment

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

这里比较高频的接口是 GetParentProgram、GetParent,可以把这两个接口规范下即可。

@gouzil gouzil changed the title [IR Dialect] ⚔Elden chapter 1.3⚔ [IR Operation] ⚔Elden chapter 1.3⚔ Jul 24, 2023
@paddle-bot paddle-bot bot added the contributor External developers label Jul 24, 2023
@luotao1 luotao1 added the HappyOpenSource 快乐开源活动issue与PR label Jul 25, 2023
@@ -227,6 +227,15 @@ Program *Operation::GetParentProgram() {
return module_op ? module_op.program() : nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

非const可以调用const的实现,可能需要对返回做一次const_cast,这样就只需要维护一份代码就可以了

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@sunzhongkai588 sunzhongkai588 left a comment

Choose a reason for hiding this comment

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

LGTM, no docs changes

@luotao1 luotao1 merged commit a147591 into PaddlePaddle:develop Aug 1, 2023
@gouzil gouzil deleted the elden_1.3 branch August 1, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers HappyOpenSource 快乐开源活动issue与PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants