Skip to content

Conversation

@jasonmnemonic
Copy link
Contributor

Added a virtual destructor.

@teemuatlut
Copy link
Owner

Can you give a bit more detail about the issue?
There's a Cpp core guideline about base class destructors that supports this type of fix but it also provides an alternative method that I think would be more appropriate here. I'm trying to move away from runtime polymorphism in favor of CRTP.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-dtor-virtual

There's also a question when would this typically be an issue on an embedded platform. Most of the time the objects get created as globals and so the destructors are never called.
When I looked into the reasons behind this issue, the examples tended to show using heap to create the object and manually calling delete on it. This type of coding tends to be frowned upon due to heap fragmentation. There's also no resource allocations made by the library, aside from sw serial object which is being removed anyway in favor of dependency injection, and so as far as I understand, there's nothing specific that the destructor would be responsible for deallocating.

@jasonmnemonic
Copy link
Contributor Author

Thank you for the reply. When you explained this, I just realised that the intention on embedded is different. I was trying to derive the code to extend my own stuff without changing your code and make use of polymorphism to make changes without affecting the base code and was testing it via new and delete in which you are spot on about heap fragmentation, etc. In actual use, there isn't going to be a new and delete all the time so that is a moot point.

Thinking aloud, I am not sure if this is a moot point or a possible scenario. Through the program run, maybe there is a need to reboot the driver so maybe a new and delete is needed but this is the only scenario that comes to my mind.

Should I close this issue?

Maybe you could add a comment on the destructor e.g. //virtual ~TMCStepper() {} // say why it is not needed so others know it is intended?

Thank you again for the great work! :-)

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.

2 participants