Fix compilation warnings

[jtentor]

Remove compilation warnings

• Add virtual destructor in HardwareSerial.h, Stream.h and Print.h
• Add attribute initialization in Stream.h

[facchinm]

Hi @jtentor , thanks for the contribution! Did you check if the addition increases the generated binary size? Second question: how did these warnings appear? Did you enable --pedantic or similar?

[jtentor]

1st question:

Yes, the addition increase the generated binary size.
Compiling this simple code with Arduino IDE

// -------------------
void setup() {
  Serial.begin(9600);
}
void loop() {
}
// -------------------

With arduino original HardwareSerial.h, Print.h and Stream.h we have:
Sketch uses 1390 bytes (4%) of program storage space. Maximum is 32256 bytes.
Global variables use 182 bytes (8%) of dynamic memory, leaving 1866 bytes for local variables. Maximum is 2048 bytes.

With the addition to fix four warnings, three for virtual destructor and one for attribute not initialized we have:
Sketch uses 2042 bytes (6%) of program storage space. Maximum is 32256 bytes.
Global variables use 196 bytes (9%) of dynamic memory, leaving 1852 bytes for local variables. Maximum is 2048 bytes.

With the addition to fix only three virtual destructor warnings we have:
Sketch uses 2034 bytes (6%) of program storage space. Maximum is 32256 bytes.
Global variables use 196 bytes (9%) of dynamic memory, leaving 1852 bytes for local variables. Maximum is 2048 bytes.

It means we need 8 bytes for attribute initialization and 644 for virtual destructor.

In this trivial code represents relatively a lot of code, but it becomes less important as code grows.

2nd question:

I am editing and compiling with Eclipse IDE for C/C++ Developers Version: Neon.3 Release (4.6.3) Build id: 20170314-1500 with Jan Baeyens plugin (http://eclipse.baeyens.it/).

Is possible Eclise have the "pedantic" option as default, but it is not set in my compilation options.

In Arduino IDE we have -MMD option wich is not set in Eclipse IDE.

The Eclipse IDE have a "Problems" windows where I found the four warnings, three for virtual destructor and one for attribute not initialized.

Possible this warnings are not relevant for hobbyist products, but I am a software engineering and checking for no warnings in my compilations is a quality factor.

Thanks @facchinm

[facchinm]

You are welcome 🙂
Adding (virtual) destructors to a couple of important base classes (like Print or Stream) have been discussed in the past, but it only increases the final binary size and they are mostly useless in embedded projects, thus the idea has been discarded. In boards like the UNO every single byte is important so adding an useless feature to make the static analyzer happy is a no-go.
About the command line, -MMD is used to generate the intermediate dependency files, so it shouldn't affect the compiler output in any way.

[jtentor]

You are correct, embedded systems do not need unnecessary code bytes.

I'm going to work on some -D options that enable or disable these virtual constructors

As soon as I have them I will share them.

Thank you.

[Ivan-Perez]

The only required virtual destructor is the one on the Print class, because HardwareSerial and Stream both extend from that class.

This doesn't only "make the static analyzer happy", it also fixes memory leaks (that's what the warning is for) on bigger projects which extend and add functionality of any of the Print subclasses.

Any way, we have discussed that before. For example, at #4466

[jtentor]

Hi @Ivan-Perez , from your question Is not it important that the library behaves correctly?

For software engineering is very important, but in small embedded systems or that who not use virtual destructor this adds unnecessary code.

Remember in first instance, Arduino is for little embedded systems; it is a stakeholder decision.

[PaulStoffregen]

fixes memory leaks ... on bigger projects which extend and add functionality of any of the Print subclasses.

Can you give some specific examples where this has been a problem?

By examples, I mean real projects and libraries. So far, every usage I've ever seen has been a statically allocated instance. Are people really using these classes with new & delete?

[Ivan-Perez]

Hi @jtentor and @PaulStoffregen, I showed an example on one comment in the PR I linked in my previous message. Here you have the link to that comment: #4466 (comment). I think it is a very simple example.

Another person show me a workaround to fix my issue, so no complains, but it would be easier to just delete the object without using such workarounds. In my project, adding the Print virtual destructor only adds a few bytes (less than 100), as I use them everywhere else.

[facchinm]

Hi @jtentor ,
we are closing all the API related PRs in this repo.
If you are still interesting in getting this merged could you reopen in https://github.com/arduino/ArduinoCore-api ? Here you can find some hints about reapplying the patch to that repo easily.
Thanks!