Small cleanup fixes #211
Small cleanup fixes #211
Conversation
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Codecov Report
@@ Coverage Diff @@
## ign-launch2 #211 +/- ##
============================================
Coverage 56.74% 56.74%
============================================
Files 2 2
Lines 326 326
============================================
Hits 185 185
Misses 141 141 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -30,6 +30,9 @@ namespace ignition | |||
| /// \brief Base class for launch plugins. | |||
| class Plugin | |||
| { | |||
| // Default destructor | |||
| public: virtual ~Plugin() {} | |||
There was a problem hiding this comment.
ABI checker says that this breaks ABI. Does that matter?
There was a problem hiding this comment.
I think it's safe to ignore that. If a class inherited from Plugin and implemented a destructor, then this will allow that destructor to be called correctly. And it won't break their code. In a similar vein, if a child did not implement a destructor then the code should also be safe.
There was a problem hiding this comment.
the size of the v-table is changed, so if there are any libraries that link against gz-launch plugins, this ABI change would break them. There probably aren't many things linking against these plugins though
There was a problem hiding this comment.
I think we can choose to ignore it but wanted to be explicit about it
🦟 Bug fix
Summary
This adds a missing
<string>header file, and declaresPlugin'svirtual destructor so that a derived class's destructor can be called properly.Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.