Return to Answer

• 87.3k
• 14
• 104
• 322

• 87.3k
• 14
• 104
• 322

The provided main() exercises the API, but doesn't actually test it. It always returns a zero (success) exit status, even if the functions fail, or don't behave as advertised.

Worse than that, it misrepresents how the functions should be used, because it fails to check for run-time error conditions they report. E.g. it blindly assumes that ll_add_last() always succeeds, instead of inspecting its return value.

Instead of the functions returning 0 for success and -1 for failure, a more intuitive interface would be to use true for success and false for failure (both values are defined in <stdbool.h>, which we have already included).

I see several problems when I attempt to compile with GCC. To begin with, we're missing <stdint.h> needed for uint8_t and stddef.h> needed for size_t. Once that's fixed, we still have a large number of warnings and errors:

gcc-12 -std=c17 -fPIC -gdwarf-4 -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wmissing-braces -Wconversion -Wstrict-prototypes -fanalyzer 284860.c -o 284860
284860.c: In function ‘ll_destroy’:
284860.c:66:17: warning: conversion from ‘size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
 66 | for(int i = list->size-1; i > 0; i--) {
 | ^~~~
284860.c: In function ‘ll_get’:
284860.c:91:23: warning: comparison of integer expressions of different signedness: ‘int’ and ‘uint32_t’ {aka ‘unsigned int’} [-Wsign-compare]
 91 | for (int i = 0; i < index; i++){
 | ^
284860.c: In function ‘ll_remove’:
284860.c:164:23: warning: comparison of integer expressions of different signedness: ‘int’ and ‘uint32_t’ {aka ‘unsigned int’} [-Wsign-compare]
 164 | for (int i = 0; i < index-1; i++) {
 | ^
284860.c: In function ‘ll_insert’:
284860.c:188:17: warning: implicit declaration of function ‘ll_add_first’ [-Wimplicit-function-declaration]
 188 | return (ll_add_first(list, data));
 | ^~~~~~~~~~~~
284860.c:196:23: warning: comparison of integer expressions of different signedness: ‘int’ and ‘uint32_t’ {aka ‘unsigned int’} [-Wsign-compare]
 196 | for (int i = 0; i < index-1; i++) {
 | ^
284860.c: At top level:
284860.c:238:5: error: conflicting types for ‘ll_add_first’; have ‘int(linked_list_t *, uint8_t)’ {aka ‘int(struct linked_list_s *, unsigned char)’}
 238 | int ll_add_first(linked_list_p list, uint8_t data) {
 | ^~~~~~~~~~~~
284860.c:238:1: note: an argument type that has a default promotion cannot match an empty parameter name list declaration
 238 | int ll_add_first(linked_list_p list, uint8_t data) {
 | ^~~
284860.c:188:17: note: previous implicit declaration of ‘ll_add_first’ with type ‘int()’
 188 | return (ll_add_first(list, data));
 | ^~~~~~~~~~~~
284860.c:265:1: error: unknown type name ‘ssize_t’; did you mean ‘size_t’?
 265 | ssize_t ll_size(linked_list_p list) {
 | ^~~~~~~
 | size_t
284860.c: In function ‘ll_size’:
284860.c:269:16: warning: conversion from ‘size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
 269 | return list->size;
 | ~~~~^~~~~~
284860.c: In function ‘ll_contains’:
284860.c:279:23: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
 279 | for (int i = 0; i < list->size; i++) {
 | ^
284860.c: In function ‘ll_first_index’:
284860.c:297:23: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
 297 | for (int i = 0; i < list->size; i++) {
 | ^
284860.c: In function ‘ll_indexes’:
284860.c:322:23: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
 322 | for (int i = 0; i < list->size; i++) {
 | ^
284860.c:327:44: warning: conversion to ‘uint32_t’ {aka ‘unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
 327 | (*indexes)[*indexes_size -1] = i;
 | ^
284860.c: In function ‘ll_print’:
284860.c:338:23: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
 338 | for (int i = 0; i < list->size; i++) {
 | ^
284860.c: In function ‘ll_print_backward’:
284860.c:349:18: warning: conversion from ‘size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
 349 | for (int i = list->size-1; i >= 0; i--) {
 | ^~~~
284860.c: In function ‘main’:
284860.c:383:29: warning: conversion from ‘size_t’ {aka ‘long unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value [-Wconversion]
 383 | ll_insert(&list, 8, list.size);
 | ~~~~^~~~~
284860.c:418:30: warning: conversion from ‘size_t’ {aka ‘long unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value [-Wconversion]
 418 | ll_insert(&list, 25, list.size);
 | ~~~~^~~~~
284860.c:427:27: warning: comparison of integer expressions of different signedness: ‘int’ and ‘uint32_t’ {aka ‘unsigned int’} [-Wsign-compare]
 427 | for (int i = 0; i < indexes_size; i++) {
 | ^
284860.c:436:27: warning: comparison of integer expressions of different signedness: ‘int’ and ‘uint32_t’ {aka ‘unsigned int’} [-Wsign-compare]
 436 | for (int i = 0; i < indexes_size; i++) {
 | ^
284860.c:446:27: warning: comparison of integer expressions of different signedness: ‘int’ and ‘uint32_t’ {aka ‘unsigned int’} [-Wsign-compare]
 446 | for (int i = 0; i < indexes_size; i++) {
 | ^
284860.c:361:14: warning: unused parameter ‘argc’ [-Wunused-parameter]
 361 | int main(int argc, char *argv[]){
 | ~~~~^~~~
284860.c:361:26: warning: unused parameter ‘argv’ [-Wunused-parameter]
 361 | int main(int argc, char *argv[]){
 | ~~~~~~^~~~~~

ll_remove_first() fails to null out the previous pointer of the new head node - the equivalent action in ll_remove_last() is present (though here, we wrongly assign to node_to_remove just after we free it).

This test looks to be in the wrong order:

 if (is_list_empty(list) || index >= list->size || list == NULL) {

If list is a null pointer, then we must not call is_list_empty() or access list->size - so we should have the list == NULL test on the left side of those || operators.

Also the is_list_empty() test is redundant. If the list is empty, then list->size is zero, which cannot be greater than the (unsigned) index.

However, we're using the wrong type for index - if the list length is a size_t, then we need to be using the same type for indexing.

We're over-testing in is_list_empty():

 if (list->head == NULL && list->tail == NULL && list->size == 0) {

Our invariant requires that head and tail pointers are null if the size is zero, so we only need to test the size:

static bool is_list_empty (linked_list_p list)
{
 return list->size == 0;
}

There's a lot of near-duplicate code, mainly due to having to treat first and last elements differently to other nodes. Consider using a dummy head node to simplify the implementation.

The type linked_list_p is harmful, as it hides a pointer. It makes it harder to write const-correct code (as we have no typedef for a linked_list_t const*). It's better to be open and transparent than to hide indirection.