-
Notifications
You must be signed in to change notification settings - Fork 515
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.
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
setFirmwareVersion leaks memory #36
Comments
Good find. The unit tests are already proving to be useful :) |
I'm a bit stuck as to what to do with this. I'd like to provide a fix at the same time as the test (so that when the test is run it passes) but to do so I'd have to create a pull request to the unit tests branch (since we don't have tests on dev). Should I do this as two separate commits in a pull request to the unit tests branch and assume that the fix commit will be applied by someone else to dev? Alternatively I could create two pull requests, one with the test and fix commit to the unit test branch and another with just the fix to dev? |
Yeah I see how that could be annoying. I'll merge unit-tests into dev. |
I merged unit-tests into dev. If you have new work in your unit-tests branch, just merge in upstream dev and then submit a pull request to merge your branch of unit-tests into dev. |
I'm going to add memory leak checking to ArduinoUnit so I don't have any new work in my unit-tests branch - I'll rewrite the above test before creating a pull request. Just to clarify, are you saying from now on push all unit tests (and fixes) directly to dev and create pull requests from there? |
Yeah since it's making it easier to detect and fix bugs. Will also be helpful in adding new features. |
Great, thanks! |
Fixed issue #36 (setFirmwareVersion leaks memory)
fixed in dev branch |
The intended usage of
setFirmwareVersion
is (I suspect) that it is called at most once, in which case the memory it allocates will be retained for the lifetime of the sketch (and therefore not leaked, even though it is never deallocated). However, ifsetFirmwareVersion
is called multiple times, previously allocated memory is not released, leading to a memory leak.Here's a unit test which demonstrates the issue (using ArduinoUnit and the MemoryFree library - from http://playground.arduino.cc/Code/AvailableMemory):
The output indicates that 20 bytes are leaked:
The text was updated successfully, but these errors were encountered: