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

rgw: eradicate dynamic memory allocation in RGWWriteRequest. #11062

Closed

Conversation

rzarzynski
Copy link
Contributor

It would be really nice to have Boost 1.56 that brought boost::optional::emplace().

CC: @mattbenjamin.

dispose_processor(processor);
processor = select_processor(*static_cast<RGWObjectCtx *>(s->obj_ctx),
&multipart);
processor = boost::none;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really needed? does boost::optional try to call RGWPutObjProcessor_Atomic::operator=() otherwise?

@cbodley
Copy link
Contributor

cbodley commented Sep 13, 2016

@rzarzynski have you looked at RGWPutObj::execute()? would the same strategy work there?

changing it now would cause conflicts with #10932, but we should get that eventually

@rzarzynski
Copy link
Contributor Author

@cbodley: I'm afraid that for RGWPutObj::execute() we would need static_ptr to handle the polymorphism. The RGWPutObj instantiates RGWPutObjProcessor_Atomic or RGWPutObjProcessor_Multipart depending on the multipart variable.

@yehudasa
Copy link
Member

yehudasa commented Oct 7, 2016

@cbodley ping

@ghost
Copy link

ghost commented Nov 17, 2016

jenkins test this please (no logs)

@cbodley
Copy link
Contributor

cbodley commented Nov 18, 2016

@rzarzynski should be able to use optional::emplace() here now?

@rzarzynski
Copy link
Contributor Author

@cbodley: yep, we should be perfectly able to do the switch now. I will send the update soon.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
@rzarzynski
Copy link
Contributor Author

@cbodley, @mattbenjamin: fixed the commit and switched to boost::optional::emplace.

@mattbenjamin
Copy link
Contributor

@rzarzynski I'm fine with using this strategy here, will test

@rzarzynski
Copy link
Contributor Author

@mattbenjamin: thanks! :-)

@oritwas
Copy link
Member

oritwas commented Apr 3, 2017

@rzarzynski please rebase

@yehudasa
Copy link
Member

@rzarzynski what's the status of this one?

@liewegas liewegas removed the needs-qa label May 8, 2017
@stale
Copy link

stale bot commented Oct 18, 2018

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If you are a maintainer or core committer, please follow-up on this issue to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Oct 18, 2018
@cbodley
Copy link
Contributor

cbodley commented Oct 18, 2018

see #24453

@cbodley cbodley closed this Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants