eteran / c-vector

A dynamic array implementation in C similar to the one found in standard C++
MIT License
737 stars 109 forks source link

add cvector_free_and_free_elements #33

Closed TomiyokaTanaka closed 2 years ago

TomiyokaTanaka commented 2 years ago

This is a handy function for freeing each elements in case they are also allocated in the heap for example like string, and only using cvector_free won't each elements from the heap

#define CVECTOR_LOGARITHMIC_GROWTH

#include "cvector.h"
#include <stdio.h>
#include <stdlib.h>
int main(int argc, char *argv[]) {
    char str_1[] = "hello world";
    char str_2[] = "good bye world";
    char str_3[] = "not today" ;
    cvector_vector_type(char*) v = NULL;

    cvector_push_back(v, strdup(str_1));
    cvector_push_back(v, strdup(str_2));
    cvector_push_back(v, strdup(str_3));

    if (v) {
        size_t i;
        for (i = 0; i < cvector_size(v); ++i) {
            printf("v[%zu] = %s\n", i, v[i]);
        }
    }

    cvector_free(v);
   // cvector_free_and_free_elements(v,free);

    return 0;
}

in the example above, valgrind tells that there are memory leak when using cvector_free instead of cvector_free_and_free_elements

I have also added test for make memcheck

eteran commented 2 years ago

Interesting idea. It's just on the edge of "in scope" for this simple vector implementation. My main hesitation is that something like this is completely normal in C++ std::vector which I am trying to model:

std::vector<char *> v;
v.push_back(strdup(s));

which will of course be a memory leak. That is, it is considered up to the user to make sure that the contents of the vector release anything they own.

That being said, I can easily imagine a second header, something like cvector_utils.h which would have things like free_each an other things like a cvector aware for_each macro to cover the common use cases like yours.

TomiyokaTanaka commented 2 years ago

Interesting idea. It's just on the edge of "in scope" for this simple vector implementation. My main hesitation is that something like this is completely normal in C++ std::vector which I am trying to model:

std::vector<char *> v;
v.push_back(strdup(s));

which will of course be a memory leak. That is, it is considered up to the user to make sure that the contents of the vector release anything they own.

That being said, I can easily imagine a second header, something like cvector_utils.h which would have things like free_each an other things like a cvector aware for_each macro to cover the common use cases like yours.

yeah, a second header for utilities are nice, I am gonna add that to my fork and submit another PR soon

TomiyokaTanaka commented 2 years ago

I have just added for_each macro and free_each into cvector_utils.h. How is this looking?

eteran commented 2 years ago

I just called it a night, I'll check it out tomorrow.

Thanks for the PR!

eteran commented 2 years ago

It's looking great. I may do some minor tweaks to it to make it match my personal style, but I plan to merge it in ๐Ÿ‘๐Ÿป

eteran commented 2 years ago

@ShiromiTempest I made some minor changes post merge.

  1. applied clang-format
  2. renamed the functions to be more inline with my style
  3. minor stylistic changes to match my style
  4. removed the technically redundant NULL checks in cvector_free_each_and_free since both operations it calls already do a NULL check

Otherwise, your code was simple and functional. Thanks!

TomiyokaTanaka commented 2 years ago

Your welcome, I am happy to help๐Ÿ‘