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

Cleanup producer_plugin pimpl idiom. #1213

Merged
merged 17 commits into from
May 30, 2023
Merged

Cleanup producer_plugin pimpl idiom. #1213

merged 17 commits into from
May 30, 2023

Conversation

greg7mdp
Copy link
Contributor

@greg7mdp greg7mdp commented May 25, 2023

Normally, for the pimpl idiom, the implementation resides in the private class, in our case producer_plugin_impl.

Before this change, many functions were defined in producer_plugin, directly accessing fields from producer_plugin_impl via the my pointer.

This PR does not change what the code does, it should be 100% equivalent to what was there before, however it moves most of the member functions implementation within the producer_plugin_impl class.

I wouldn't mind doing a even fuller cleanup, so that all the producer_plugin methods forward to producer_plugin_impl and are grouped together at the bottom of the file, but it might make the change even harder to follow. Please let me know what you think.

PS: most of the changes are white space changes, to make formatting consistent within this file. You make benefit from appending ?w=1 to the diff window url to limit whitespace differences.

@greg7mdp greg7mdp requested review from heifner and vladtr May 25, 2023 21:27
plugins/producer_plugin/producer_plugin.cpp Show resolved Hide resolved
plugins/producer_plugin/producer_plugin.cpp Outdated Show resolved Hide resolved
plugins/producer_plugin/producer_plugin.cpp Outdated Show resolved Hide resolved
@greg7mdp greg7mdp requested review from ScottBailey and removed request for ScottBailey May 30, 2023 16:06
@greg7mdp greg7mdp merged commit f53faff into main May 30, 2023
@greg7mdp greg7mdp deleted the producer_plugin_cleanup branch May 30, 2023 22:12
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.

3 participants