Singly-linked implementation in C

Question

I'm a C++ programmer and thought I'd revisit the roots, so I scribbled something really quickly. I would appreciate input on how my C code is looking!

slist.h

#ifndef SLIST_H
#define SLIST_H

#include <stddef.h>
#include <stdbool.h>

struct slist_s 
{
    int key;
    struct slist_s* next;
};

typedef struct slist_s slist_t;

slist_t* slist_push_front(slist_t*, int);
slist_t* slist_push_back(slist_t*, int);

slist_t* slist_pop_front(slist_t*);
slist_t* slist_pop_back(slist_t*);

_Bool    slist_empty(const slist_t* const);
slist_t* slist_clear(slist_t*);

size_t   slist_size(const slist_t*);

void     slist_print(const slist_t*, const char*);

slist_t* slist_for_each(slist_t* const, void(*)(int*));

#endif 

slist.c

#include <stdlib.h>
#include <stdio.h>
#include <stddef.h>
#include <stdbool.h>

#include "slist.h"

slist_t* slist_push_front(slist_t* root, int val)
{
    slist_t* node = malloc(sizeof(slist_t));
    if (!node) {
        return NULL;
    }
    node->key = val;
    node->next = !root ? NULL : root;   

    root = node;
    return root;
}

slist_t* slist_push_back(slist_t* root, int val)
{
    slist_t* node = malloc(sizeof(slist_t));
    if (!node) {
        return NULL;
    }
    node->key = val;

    if (!root) 
    {   /* list is empty */
        node->next = NULL;
        root = node;
    }
    else 
    {   /* need to walk to end of list */
        slist_t* temp = root;
        while (temp->next) {
            temp = temp->next;
        }
        node->next = NULL;
        temp->next = node;
    }
    return root;
}

slist_t* slist_pop_front(slist_t* root)
{
    if (!root) {
        return NULL;
    }
    if (!root->next) 
    {
        free(root);
        return NULL;
    }
    slist_t* todel = root;
    root = root->next;
    free(todel);
    return root;
}

slist_t* slist_pop_back(slist_t* root)
{
    if (!root) {
        return NULL;
    }
    if (!root->next)
    {
        free(root);
        return NULL;
    }
    slist_t* todel = root;
    while (todel->next->next) {
        todel = todel->next;
    }
    free(todel->next);
    todel->next = NULL;
    return root;
}

_Bool slist_empty(const slist_t* const root)
{
    return root;
}

slist_t* slist_clear(slist_t* root)
{
    while (root) {
        root = slist_pop_front(root);
    }
    return root;
}

size_t slist_size(const slist_t* root)
{
    size_t size = 0;
    const slist_t* temp = root;
    while (temp) 
    {
        temp = temp->next;
        ++size;
    }
    return size;
}

void slist_print(const slist_t* root, const char* format)
{
    const slist_t* temp = root;
    while (temp) 
    {
        printf(format, temp->key);
        temp = temp->next;
    }
}

slist_t* slist_for_each(slist_t* const root, void(*fun)(int*))
{
    slist_t* temp = root;
    while (temp)
    {
        fun(&temp->key);
        temp = temp->next;
    }
    return root;
}

Answer 1

• Returning root

  Returning the root suggests the use case

      root = slist_push_front(root, value);
  

  However, if node allocation fails, the return value is NULL, and the whole list is lost. I recommend to change the signature to

      int slist_push_front(slist_t ** rootptr, int value);
  

  to be used along the lines of

      if (slist_push_front(&root, value) == FAIL) {
          ....
      }
  

---

• In slist_push_front, it doesn't matter whether the root is NULL or not. node->next = root will work just fine in both cases.

• In slist_push_back the next pointer must become null in any case, so it seems logical to assign it once when the node is created.

  I also recommend to refactor the while (temp->next) loop into a slist_get_tail function. Then slist_push_back is streamlined into

  int slist_push_back(slist_t ** root, int val)
  {
      slist_t * node = slist_create_node(val);
      if (node == NULL) {
          return FAILURE;
      }
      slist_t * tail = slist_get_tail(*root);
      if (tail) {
          tail->next = node;
      } else {
          *root = node;
      }