fix(Print): add missing virtual destructor - #281
Open
saikumar-mandaji wants to merge 1 commit into
Open
Conversation
Print is an abstract base class (virtual size_t write(uint8_t) = 0) used polymorphically throughout the core: any Stream/HardwareSerial/ etc. is routinely held and deleted through a Print* or Stream*. With no virtual destructor, 'delete'-ing such an object through a base pointer is undefined behavior -- the derived class's destructor (and therefore its members' destructors) never runs. This isn't hypothetical: it was already observed producing a real -Wdelete-non-virtual-dtor warning in a shipping Arduino core (R4's WiFiS3 Modem.cpp, deleting a UART through a Serial/Print-derived pointer), see arduino#256. Fix: add 'virtual ~Print() { }', matching the inline empty-body style already used for this exact purpose on other polymorphic base classes in this repo (CanMsg, HardwareCAN, HardwareSPI). Since Print already has a pure virtual write() and therefore already carries a vtable pointer in every instance, this adds no per-instance memory cost on 8-bit AVR or any other target -- only one additional vtable entry. ## Verification Compiled a minimal Stream-derived class with 'arm-none-eabi-g++ -std=gnu++14 -Wall -Wextra -Wdelete-non-virtual-dtor' (no host-native compiler was available in this environment): before this change, 'delete'-ing it through a Print* triggers -Wdelete-non-virtual-dtor; after, the warning is gone. Also confirmed a Print-derived class still compiles cleanly with the added destructor. Did not run ArduinoCore-API's own host test suite (it needs a native g++/clang, not present here). Fixes arduino#256
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
Printis an abstract base class (virtual size_t write(uint8_t) = 0) used polymorphically throughout the core: anyStream/HardwareSerial/etc. is routinely held and deleted through aPrint*orStream*. With no virtual destructor,delete-ing such an object through a base pointer is undefined behavior — the derived class's destructor (and therefore its members' destructors) never runs.This isn't hypothetical: it was already observed producing a real
-Wdelete-non-virtual-dtorwarning in a shipping Arduino core (Renesas Uno R4'sWiFiS3/src/Modem.cpp, deleting aUARTthrough aSerial/Print-derived pointer) — see #256.Fix
Add
virtual ~Print() { }, matching the inline empty-body style already used for this exact purpose on other polymorphic base classes in this repo (CanMsg,HardwareCAN,HardwareSPI).Since
Printalready has a pure virtualwrite()and therefore already carries a vtable pointer in every instance, this adds no per-instance memory cost on 8-bit AVR or any other target — only one additional vtable entry (a few bytes in flash).Verification
Compiled a minimal
Stream-derived class witharm-none-eabi-g++ -std=gnu++14 -Wall -Wextra -Wdelete-non-virtual-dtor(no host-native compiler was available in this environment): before this change,delete-ing it through aPrint*triggers-Wdelete-non-virtual-dtor; after, the warning is gone. Also confirmed aPrint-derived class still compiles cleanly with the added destructor.Did not run this repo's own host test suite (needs a native g++/clang, not present in this environment) — flagging that honestly rather than claiming a full test-suite pass.
Fixes #256