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.

Sign up for GitHub

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

Jump to bottom

Added missing virtual destructors #218

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+4 −1
Open

Added missing virtual destructors #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/Print.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ class Print
void setWriteError(int err = 1) { write_error = err; }
public:
Print() : write_error(0) {}
virtual ~Print() {}


int getWriteError() { return write_error; }
void clearWriteError() { setWriteError(0); }
Expand Down
1 change: 1 addition & 0 deletions api/Printable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class Print;
class Printable
{
public:
virtual ~Printable() {}
virtual size_t printTo(Print& p) const = 0;
};

Expand Down
2 changes: 1 addition & 1 deletion api/String.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class String
explicit String(unsigned long, unsigned char base=10);
explicit String(float, unsigned char decimalPlaces=2);
explicit String(double, unsigned char decimalPlaces=2);
~String(void);
virtual ~String(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you think that String warrants a virtual destructor? Is there any virtual function in the class? On a quick glance I could not find one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This question answer is as simple as understanding if we want to allow the user to extend the arduino::String class

Copy link

Choose a reason for hiding this comment

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

This question answer is as simple as understanding if we want to allow the user to extend the arduino::String class

Undefined behaviour is related to having any virtual function, not to inheritance.
So currently, we do need to add a virtual destructor to Print because it has a virtual functions.

Thus, a virtual destructor is not currently a necessity for String. I can inherit from String, ex. MyString and use MyString all along my code, without any undefined behaviour.

However, you might want to add a virtual destructor to String because you expect users to add virtual functions to their MyString. But that's more about securing the code about expected usages than fixing the current code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can have memory leaks if in MyString class I allocate something that needs to be deleted, I create a proper destructor, I reference it with a pointer to String then I delete it, the derived class destructor is not called.

The answer, in my opinion, is not strictly limited to "String doesn't have virtual methods", because I could want some features of String class and extend them with my custom features. I think you can implement a solution in other better ways, like by embedding String instead. Thus my question: do we want to allow the user to extend String class?


// memory management
// return true on success, false on failure (in which case, the string
Expand Down
Loading