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 memory leak on iterator/list destroy #41

Conversation

vladhrapov
Copy link

@vladhrapov vladhrapov commented Aug 29, 2022

Fixes memory leak on objects memory freeing. Pass pointer to pointer to release memory inside an actual pointer, but not a copy.

Fixes issue #17.

@diasbruno
Copy link
Member

diasbruno commented Aug 29, 2022

It seems that there are no "mem leaks" on tests on master. Is there something I'm missing?
Or is this PR to help setting the passed pointer to NULL?

[nix-shell:/usr/local/src/list]$ valgrind ./bin/test 
==41378== Memcheck, a memory error detector
==41378== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==41378== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==41378== Command: ./bin/test
==41378== 

list_t: 40
list_node_t: 24
list_iterator_t: 16

... list_node_new
... list_rpush
... list_lpush
... list_find
... list_at
... list_remove
... list_rpop
... list_lpop
... list_destroy
... list_empty_list_destroy
... list_destroy_complexver
... list_iterator_t
... 100%

==41378== 
==41378== HEAP SUMMARY:
==41378==     in use at exit: 0 bytes in 0 blocks
==41378==   total heap usage: 82 allocs, 82 frees, 3,128 bytes allocated
==41378== 
==41378== All heap blocks were freed -- no leaks are possible
==41378== 
==41378== For lists of detected and suppressed errors, rerun with: -s
==41378== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

@vladhrapov
Copy link
Author

It was for helping with both, but yeah, nvm, just double checked no leak there. Also checked that pointer was not reset as you said, so anyway helps to fix that case.

@diasbruno
Copy link
Member

Looks good. It will need to bump a major version since it changes the api.
cc @stephenmathieson @jwerle see if it's ok...

@zachmann
Copy link

I'm against this change. It effectively only sets the passed pointer to NULL, but IMO makes the api (unnecessarily) more complex.

If @vladhrapov really needs to set the pointer the NULL after a free, this can be done with a simple macro:

#ifndef free_list
#define free_list(ptr) \
  do {               \
    list_destroy((ptr)); \
    (ptr) = NULL;    \
  } while (0)
#endif  // free_list

Of course you can choose a better name.

It would also be possible to include such macros into list.h; I think that would be the better approach than passing a list_t**, but IMHO I think that should not be necessary.

@vladhrapov
Copy link
Author

Makes sense @zachmann, closing this PR then.

@vladhrapov vladhrapov closed this Aug 31, 2022
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.

3 participants