4
\$\begingroup\$

I am new to C and have been learning it for about a month now.

This is my attempt to implement a dynamic array. My code works, but i don't know whether or not it leaks memory.

But my code works as I expected. Is my code good or bad?

#include <stdio.h>#include <stdlib.h>#include <time.h>typedef struct{    int size;    int arr[5];} dynamic_array;dynamic_array *create_dynamic_array(int initial_value){    dynamic_array *self = (dynamic_array *)calloc(1, sizeof(dynamic_array));    if (self == NULL)    {        printf("\nCan not allocate memory\n");        exit(0);    }    if (initial_value == NULL)        return self;    self->size = 1;    self->arr[0] = initial_value;    return self;}void print_dynamic_array(dynamic_array *self){    for (int i = 0; i < self->size; i++)    {        printf("\nval=%d,index=%d", self->arr[i], i);    }    printf("\n length of arr =%d", self->size);}void push(dynamic_array *self, int v){    int len = self->size;    self->arr[len] = v;    self->size = (len + 1);}int *get_element(dynamic_array *self, int index){    if (index >= self->size)    {        return NULL;    }    return &self->arr[index];}void remove(dynamic_array *self, int index){    int *address = get_element(self, index);    int len = self->size, i;    for (i = 0; i < (self->size - index); i++)    {        *(address + i) = *(address + i + 1);    }    self->size = (len - 1);    printf("\nbrooooo %p,val=%d\n", (address + i + 1), *(address + i + 1));}int length(dynamic_array *self){    return self->size;}void freeDynamicArray(dynamic_array *da){    //free(da->arr);    free(da);}int main(){    time_t t;    srand((unsigned)time(&t));    dynamic_array *DA = create_dynamic_array(NULL);    print_dynamic_array(DA);    for (int i = 0; i < 15; i++)    {        push(DA, (rand() % 100) + 1);    }    print_dynamic_array(DA);    printf("\n element at 4 is %d", *get_element(DA, 4));    remove(DA, 7);    print_dynamic_array(DA);    freeDynamicArray(DA);    return 0;}
Mast's user avatar
Mast
13.9k12 gold badges57 silver badges128 bronze badges
askedFeb 14, 2024 at 17:47
Logeshwaran K's user avatar
\$\endgroup\$
2
  • 2
    \$\begingroup\$How are you avoiding to get out of the fixed boundaries of thatdynamic array? It isn't dynamic.\$\endgroup\$CommentedFeb 14, 2024 at 18:34
  • 1
    \$\begingroup\$(Tiny comment not big enough to be an answer:)dynamic_array is an unfortunately long name for a datatype, imo.\$\endgroup\$CommentedJun 26 at 21:57

3 Answers3

11
\$\begingroup\$

Your code does not implement a dynamic array. It never extends its memory when elements are added, so it overflows its bounds after a number of insertions, and proceeds to overwrite memory it doesn't own - that's called aBuffer Overflow and can be even more serious than a mere leak.

A true dynamic array needs to userealloc() (and aflexible array member) to provision more memory when elements are added.


There are some things that make no sense:

if (initial_value == NULL)

It doesn't make sense to compare an integer against a pointer - what are you trying to achieve here?

Similarly, why are we trying to pass a pointer as the initial value when we create the array, here?:

dynamic_array *DA = create_dynamic_array(NULL);

I think it's best to get rid of the "initial value" business, and always create an empty collection.

Then we can usemalloc() rather thancalloc() since we assign to both members of the struct.


Other issues:

dynamic_array *self = (dynamic_array *)calloc(1, sizeof(dynamic_array));

A good practice is to usesizeof *self rather than writing the type (it makes it easier to change the type later, and can help reviewers when the assignment is far from the declaration). And the cast is unnecessary in C, becausevoid* converts to any kind of object pointer.

So that should be:

dynamic_array *self = calloc(1, sizeof *self);
if (self == NULL){    printf("\nCan not allocate memory\n");    exit(0);

Good work considering the error case. Error messages really ought to go to the error stream (fprintf(stderr, …)). Actually, it's probably better just to return the null pointer, so the caller can decide how to report the error:

if (!self) {    return self;}

We certainly shouldn't be exiting with status 0, which is thesuccess value.

void remove(dynamic_array *self, int index)

We can't use that name -remove() is already a function in<stdio.h>. I propose using a naming system where all our functions begin withdynarr_ so we know they belong together. So this would bedynarr_remove().

for (i = 0; i < (self->size - index); i++){    *(address + i) = *(address + i + 1);}

The normal way to write the assignment is using the index operator:address[i] = address[i+1]. That said, it's unnecessary: this loop can be replaced by a call tomemmove().

printf("\nbrooooo %p,val=%d\n", (address + i + 1), *(address + i + 1));

We need to convert the pointer tovoid* to print it.

dynamic_array *DA = dynarr_create();

We usually use all-caps names for preprocessor macros (because they are dangerous, and don't behave like C identifiers). Keep local variables in lowercase.

answeredFeb 14, 2024 at 18:41
Toby Speight's user avatar
\$\endgroup\$
6
  • \$\begingroup\$fun fact:struct { int size; int arr[]; } is valid C: the array is called a "flexible array member" and you can malloc/realloc a larger size, supporting use-cases like this where you have some fixed members and a variable-sized array at the end.en.wikipedia.org/wiki/Flexible_array_member I think this is only valid if the array size in the struct declaration is empty or0, though, so you couldn't just addrealloc to the OP's existing code, you also need to change the declaration.\$\endgroup\$CommentedFeb 15, 2024 at 15:56
  • \$\begingroup\$I should have mentioned flexible array members - I've incorporated your link. Prior to flexible array members, it was common to have a 1-element array at the end of a struct, and Ithink that's valid (but the size computation is harder to read).\$\endgroup\$CommentedFeb 15, 2024 at 16:25
  • 1
    \$\begingroup\$Back in the old days before C99, compilers didn't do nearly as much optimization based on the assumption of no UB. So back then, it would happen to work to write past the end of a 1-element array. But if supercat is correct in comments on (Array of size 0 at the end of struct), a non-zero size makes it a normal array so access past the end is UB whether or not its the last member. A compiler could assume thatp->arr[n] actually accessesp->arr[0] and thatn is0, because any other value would have UB.\$\endgroup\$CommentedFeb 15, 2024 at 16:44
  • \$\begingroup\$The spec seems to be a bit ambiguous and potentially misleading in the case of a regular array with a specific size. Evaluating an array subscriptionarr[n] is UB ifarr+n points past the end of an 'array object' thatarr points to. The thing is that an 'object' is a 'region of data storage' andmalloc() returns an 'object'. So 'arr' might not actually point to an 'array object' of some size, but rather to somewhere inside a bigger 'object'. If it's UB, things get bizarre when casting tochar *. Maybe this deserves it's own question on StackOverflow, if one doesn't exists already.\$\endgroup\$CommentedFeb 16, 2024 at 18:42
  • \$\begingroup\$FYI, this question is essentially about this issue:stackoverflow.com/questions/43736595/… Andzwol's comment points out exactly what I mentioned above.\$\endgroup\$CommentedFeb 16, 2024 at 19:03
3
\$\begingroup\$

A few points less important than the one Toby Speight made in his excellent answer.

Naming Conventions

You havecreate_dynamic_array() butfreeDynamicArray(). Then, you name functionslength() andpush(), which are likely to collide with identifiers in the rest of the program if you try to re-use this code.

Since C doesn’t have namespaces, best practice is to pick choose a convention likecamelCase orsnake_case, then pick a prefix for every public function in the module, for exampledyn_arr_ (dyn_arr_c_ for functions that take aconst dynamic_array* as the first argument,DYN_ARR_ for macros defined by the package). It is much more reasonable to warn other coders not to use names that start withdyn_arr_ than to break code that uses the identifierlength.

Allocation

This library only supports creating dynamic arrays on the heap, so you cannot store them in data structures or create local dynamic arrays.

There is one potential advantage of this: you could in theory declaredynamic_array as an incomplete type in the header, likeFILE, which can hide implementation details. That’s not the trade-off I think I would make with a dynamic array in C, but it’s valid. I suggest you mostly want shortinline functions.

I recommend you take a leaf out of POSIX’s book and define a macro like

#define DYN_ARR_INITIALIZER ((dynamic_array)\    {.size = 0U, .capacity = 0U, .data = NULL})

This is similar toPTHREAD_MUTEX_INITIALIZER orPTHREAD_COND_INITIALIZER, and lets you declare

dynamic_array foo = DYN_ARR_INITIALIZER;

You might even consider making the other initialization functions require a valid initialization like this, for example,

dynamic_array foo = DYN_ARR_INITIALIZER;dynamic_array bar = DYN_ARR_INITIALIZER;dynamic_array_fill(&foo, &source, 8, sizeof(source));dyn_array_resize(&foo, 42);dyn_array_swap(&bar, &foo);

This adds a tiny amount of overhead to initialization. In my opinion, it’s worth the cost because it forces any overwrite of adynamic_array to free the target. If there were a function that directly overwrote an uninitializeddynamic_array, the compiler would also allow it to overwrite aninitializeddynamic_array, and leak memory.

All initial allocations would then userealloc(). If the array was previously empty, this passesNULL as the previous address torealloc(), which makes it behave likemalloc(), You could then optionally zero out the newly-allocated bytes withmemset().

answeredJun 25 at 3:29
Davislor's user avatar
\$\endgroup\$
2
\$\begingroup\$

Aside from the other issues noted, the following function is needlessly verbose.

void push(dynamic_array *self, int v){    int len = self->size;    self->arr[len] = v;    self->size = (len + 1);}

Utilizing the post-increment operator the three lines in this becomeone line, removing the opportunity to mutatelen unintentionally between uses, while remaining completely understandable and idiomatic.

void push(dynamic_array *self, int v){    self->arr[self->size++] = v;}

Of course, the potential for UB remains, since no bounds checking is performed.

answeredJun 24 at 5:25
Chris's user avatar
\$\endgroup\$
4
  • \$\begingroup\$the three lines in this become one line What benefit comes from cramming as much code into as few lines as possible?\$\endgroup\$CommentedJun 26 at 19:13
  • \$\begingroup\$@AndrewHenle Use of concise, idiomatic statements or blocks is, for experienced readers, more expressive. Also, in this case, this function may not be aware thatself->size is actually along, or evenunsigned long, or something else, opening the door to a "hard-to-recognize" bug. (Let's overlooksize_t here.) There are more "fabricated scenarios" that also 'back-up' the code improvement suggested in this answer. Finding a mature balance and using judgement comes with experience... This is not "as much code..." This is1 idiomatic C statement replacing3 statements.\$\endgroup\$CommentedJun 26 at 21:42
  • \$\begingroup\$@Fe2O3 My question was meant to cut past the "less lines is better" tone of this answer. How does repeating "idiomatic" answer my question? Are you trying to claim "idiomatic" automatically means "better"? Nevermind your entire line of thinking relies entirely on "for experienced readers, more expressive". How many questions per day get posted online regarding bugs caused by misuse of the++ and-- operators? Unthinkingly writing "idomatic" code because it's "idiomatic" ignores all of that.\$\endgroup\$CommentedJun 27 at 15:43
  • \$\begingroup\$@AndrewHenle My ENTIRE thinking?? Did you not see the preamble about elimination of variablelen that was a maintenance point and invitation to a bug? You'll see what you want to see, so that's it for me... Cheers! :-)\$\endgroup\$CommentedJun 27 at 19:00

You mustlog in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.