Skip to main content
Spelling fix
Source Link
Toby Speight
  • 88.3k
  • 14
  • 104
  • 327

An obvious limitation is that that this list can only be used for entries that can be represented as a pair of int. Making a more generic linked list is a harder exercise.

Some of the names are very general, so likely to collide with user's code. This could be avoided by prefixing them with something particular to your implementation (e.g. sll_ for "singly-linked list").

<assert.h> is included but never used.

Some functions would benefit from const types - e.g. get() should not be modifying the list's content, so should probably be get(LinkedList const* list, int index). (BTW, index is a misleading name there, because it usually means the position in the list, rather than part of the element's value.

The checking of allocation functions is flawed - error messages should go to the error stream (stderr), and we must not use the null pointer:

        Node* p_head_node = malloc(sizeof *p*p_head_node);
        if (!p_head_node) {
            fprintf(stderr, "Memory allocation failed.");
            return;
        }

This is still problematic - we should tell the calling program whether or not we succeeded, and let it choose whether and how to notify the user.

get() and set() functions can reach the end of the list, which causes undefined behaviour as they attempt to dereference a null pointer.

There's a bug in free_linked_list(): we need to assign current = current->next before we free(node_to_be_freed) (running under Valgrind will help identify such use-after-free errors).

An obvious limitation is that that this list can only be used for entries that can be represented as a pair of int. Making a more generic linked list is a harder exercise.

Some of the names are very general, so likely to collide with user's code. This could be avoided by prefixing them with something particular to your implementation (e.g. sll_ for "singly-linked list").

<assert.h> is included but never used.

Some functions would benefit from const types - e.g. get() should not be modifying the list's content, so should probably be get(LinkedList const* list, int index). (BTW, index is a misleading name there, because it usually means the position in the list, rather than part of the element's value.

The checking of allocation functions is flawed - error messages should go to the error stream (stderr), and we must not use the null pointer:

        Node* p_head_node = malloc(sizeof *p);
        if (!p_head_node) {
            fprintf(stderr, "Memory allocation failed.");
            return;
        }

This is still problematic - we should tell the calling program whether or not we succeeded, and let it choose whether and how to notify the user.

get() and set() functions can reach the end of the list, which causes undefined behaviour as they attempt to dereference a null pointer.

There's a bug in free_linked_list(): we need to assign current = current->next before we free(node_to_be_freed) (running under Valgrind will help identify such use-after-free errors).

An obvious limitation is that that this list can only be used for entries that can be represented as a pair of int. Making a more generic linked list is a harder exercise.

Some of the names are very general, so likely to collide with user's code. This could be avoided by prefixing them with something particular to your implementation (e.g. sll_ for "singly-linked list").

<assert.h> is included but never used.

Some functions would benefit from const types - e.g. get() should not be modifying the list's content, so should probably be get(LinkedList const* list, int index). (BTW, index is a misleading name there, because it usually means the position in the list, rather than part of the element's value.

The checking of allocation functions is flawed - error messages should go to the error stream (stderr), and we must not use the null pointer:

        Node* p_head_node = malloc(sizeof *p_head_node);
        if (!p_head_node) {
            fprintf(stderr, "Memory allocation failed.");
            return;
        }

This is still problematic - we should tell the calling program whether or not we succeeded, and let it choose whether and how to notify the user.

get() and set() functions can reach the end of the list, which causes undefined behaviour as they attempt to dereference a null pointer.

There's a bug in free_linked_list(): we need to assign current = current->next before we free(node_to_be_freed) (running under Valgrind will help identify such use-after-free errors).

Source Link
Toby Speight
  • 88.3k
  • 14
  • 104
  • 327

An obvious limitation is that that this list can only be used for entries that can be represented as a pair of int. Making a more generic linked list is a harder exercise.

Some of the names are very general, so likely to collide with user's code. This could be avoided by prefixing them with something particular to your implementation (e.g. sll_ for "singly-linked list").

<assert.h> is included but never used.

Some functions would benefit from const types - e.g. get() should not be modifying the list's content, so should probably be get(LinkedList const* list, int index). (BTW, index is a misleading name there, because it usually means the position in the list, rather than part of the element's value.

The checking of allocation functions is flawed - error messages should go to the error stream (stderr), and we must not use the null pointer:

        Node* p_head_node = malloc(sizeof *p);
        if (!p_head_node) {
            fprintf(stderr, "Memory allocation failed.");
            return;
        }

This is still problematic - we should tell the calling program whether or not we succeeded, and let it choose whether and how to notify the user.

get() and set() functions can reach the end of the list, which causes undefined behaviour as they attempt to dereference a null pointer.

There's a bug in free_linked_list(): we need to assign current = current->next before we free(node_to_be_freed) (running under Valgrind will help identify such use-after-free errors).