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.

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

fix: destroy_files function memory leak #4

Merged
merged 5 commits into from Mar 12, 2023

Conversation

johanneshorner
Copy link
Contributor

@johanneshorner johanneshorner commented Mar 12, 2023

This pr should make the destroy_files function free the memory correctly, that was earlier allocated in the workspace_files function.

Automatic tests with memory analysis could be very useful. Haven't actually set up something like that on my own, but i can look into it.

@vhyrro Maybe you can give your opinion on the todos i added. If I were to use this library in my application, I think I would want the library to crash if I passed in a value that didn't really make sense.

@vhyrro
Copy link
Member

vhyrro commented Mar 12, 2023

First of all, thanks!

Automatic tests with memory analysis could be very useful. Haven't actually set up something like that on my own, but i can look into it.

Yeah, this would be super cool. I can try setting something up like this in a bit.

Maybe you can give your opinion on the todos i added. If I were to use this library in my application, I think I would want the library to crash if I passed in a value that didn't really make sense.

I agree that it would be nice to crash with a message in the create_* functions and all other functions that allocate or create new objects, but I don't believe we should crash in the destroy functions. To quote one of the points in the zig zen: "Resource allocation may fail; resource deallocation must succeed".

@johanneshorner
Copy link
Contributor Author

That zig zen quote does make sense. I added asserts in the create_* functions.

@johanneshorner johanneshorner marked this pull request as ready for review March 12, 2023 16:17
@vhyrro vhyrro changed the title fix destroy_files function memory leak fix: destroy_files function memory leak Mar 12, 2023
@vhyrro vhyrro merged commit 92f028f into nvim-neorg:main Mar 12, 2023
1 check passed
@vhyrro
Copy link
Member

vhyrro commented Mar 12, 2023

Thank you yet again :D

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.

None yet

2 participants