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

Added private data pointer for storage #19

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sauk2
Copy link

@sauk2 sauk2 commented Jan 9, 2024

🎉 New feature

Summary

The system plugin often needs to save static values to function, or store variables extracted from the SDF file. Implemented a class in FullSystem plugin and created a variable of that class as a unique pointer (following PImpl technique).

Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
@quarkytale quarkytale linked an issue Apr 22, 2024 that may be closed by this pull request
@quarkytale
Copy link
Collaborator

LGTM but would defer to @mjcarroll on this one.

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

I'm not opposed to this, but it isn't strictly necessary.

In many places inside of Gazebo, you will find the PIMPL pattern because we are strict about maintaining ABI compatibility within a certain version. If this ABI-compatibility isn't necessary in your project's system implementation, then it is perfectly fine to use traditional class members, rather than the PIMPL pattern.

Can you add some documentation to this effect?

Comment on lines +60 to +63
FullSystem::~FullSystem()
{
// Do nothing
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FullSystem::~FullSystem()
{
// Do nothing
}
FullSystem::~FullSystem() = default;

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.

Addition of a private member to store data
3 participants