Skip to main content
Oops, edited the wrong post
Source Link
G. Sliepen
  • 69.2k
  • 3
  • 74
  • 180
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

Here are some things that may help you improve your code.

Don't rely on non-standard extensions

Some of your code, such as the alx_mallocarrays macro is relying on a braced-group within an expression which is not valid C, even if your compiler supports it. See this question for details. The code also requires __auto_type and __attribute__ which are also gcc extensions. All of these make your code non-portable; at the very least this limitation should be expressly acknowledged in the header and/or documentation.

Use include guards

There should be an include guard in each .h file. That is, start the file with:

#ifndef LINKED_LIST_H
#define LINKED_LIST_H
// file contents go here
#endif // LINKED_LIST_H

The use of #pragma once is a common extension, but it's not in the standard and thus represents at least a potential portability problem. See SF.8

Avoid relative paths in #includes

Generally it's better to omit relative path names from #include files and instead point the compiler to the appropriate location.

#include "libalx/extra/alx/linked-list.h"
#include <stdlib.h>
#include <string.h>
#include "libalx/base/stdlib/alloc/mallocarrays.h"
#include "libalx/base/stdlib/alloc/mallocs.h"
#include "libalx/base/stdlib/alloc/reallocs.h"

For gcc, you'd use -I. This makes the code less dependent on the actual file structure, and leaving such details in a single location: a Makefile or compiler configuration file. The order of these also suggests the next item.

Put your own #includes first

If you put your own #includes first, you will catch errors in which the #include is incomplete. For example, I suspect that the three last .h files above need one or more things from <stdlib.h> or <string.h>. If that's the case, then the files that need them should #include them. Otherwise the code is dependent on the order of the #includes in the code which is a recipe for disaster and frustration.

Avoid goto

The use of goto is error prone and is better avoided. In the cases in which it's used, it's easily avoided. For example instead of this:

    if (alx_mallocs(&node->data, size))
        goto err;

    memcpy(node->data, data, size);
    node->prev    = list->current->prev;
    node->next    = list->current;

    list->current->prev->next    = node;
    list->current->prev    = node;
    list->current        = node;
    (list->nmemb)++;

    return    0;
err:
    free(node);
    return    -2;

Write this:

if (!alx_mallocs(&node->data, size)) {

    memcpy(node->data, data, size);
    node->prev    = list->current->prev;
    node->next    = list->current;

    list->current->prev->next    = node;
    list->current->prev    = node;
    list->current        = node;
    (list->nmemb)++;

    return    0;
}
free(node);
return    -2;

Eliminate "magic numbers"

There are a few numbers in the code, such as -1 and -2 that have a specific meaning in their particular context. By using named constants such as err_mallocarrays and err_mallocs, the program becomes easier to read and maintain.

Use const where practical

Some of the functions, such as alx_llist_find do not alter the passed parameters. Those parameters should be declared const.

Consider documenting the header file

The header is where I'd look to figure out how to use this class. Because the nameing of functions is generally good, I wouldn't need a lot, but some functions such as alx_llist_find and alx_llist_remove_last are a bit strange. I'd normally expect to be able to find by value rather than address and the alx_llist_remove_last seems too specialized for a general interface. Use it internally only if it's useful, but don't clutter the public interface with unneeded functions. An ideal interface is minimal but sufficient.