Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[WIP] Remove redundant shared_ptr for exception_ptr #13933

Closed
wants to merge 1 commit into from

Conversation

yuxihu
Copy link
Member

@yuxihu yuxihu commented Jan 18, 2019

std::exception_ptr is already reference counted. There is no need to wrap it with shared_ptr. This PR removes the redundant shared_ptr for var_exception and opr_exception members.

Copy link
Contributor

@larroy larroy left a comment

Choose a reason for hiding this comment

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

LGTM

@yuxihu yuxihu changed the title Remove redundant shared_ptr for exception_ptr [WIP] Remove redundant shared_ptr for exception_ptr Jan 19, 2019
@stu1130
Copy link
Contributor

stu1130 commented Jan 19, 2019

@mxnet-label-bot add [pr-awaiting-review]
@yuxihu Thanks for your contribution

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Jan 19, 2019
Copy link
Contributor

@ChaiBapchya ChaiBapchya left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Looks like CI is failing on multiple errors!

@yuxihu
Copy link
Member Author

yuxihu commented Jan 22, 2019

@mxnet-label-bot update [pr-work-in-progress]

@marcoabreu marcoabreu added pr-work-in-progress PR is still work in progress and removed pr-awaiting-review PR is waiting for code review labels Jan 22, 2019
@pinaraws
Copy link

@yuxihu Any updates?

@yuxihu
Copy link
Member Author

yuxihu commented Mar 20, 2019

@pinaraws Have not got a chance to look into the test failure. @anirudh2290 had explanation about why we are using shared_ptr around exception_ptr here. I think this PR is no longer needed. Closing it.

@yuxihu yuxihu closed this Mar 20, 2019
@yuxihu yuxihu deleted the ex_ptr branch March 20, 2019 01:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-work-in-progress PR is still work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants