1

When writing functions in C, what considerations govern the way in which results are returned from the function? My question arises from reading the following code, which creates a node in a doubly-linked list, and which comes from this book:

typedef struct DLLIST
{
    uint16 tag;                 /* This node's object type tag */
    struct DLLIST *next;
    struct DLLIST *previous;
    void *object;
    uint32 size;                /* This node's object size */
} DLLIST;

DLLIST *dlCreate(uint16 tag, void *object, uint32 size)
{
    DLLIST *newNode;

    newNode = malloc(sizeof(*newNode));

    if (newNode != NULL)
    {
        newNode->previous = NULL;
        newNode->next = NULL;
        newNode->tag = tag;
        newNode->size = size;
        newNode->object = malloc(size);

        if (newNode->object != NULL)
        {
            memcpy(newNode->object, object, size);
        }
        else
        {
            free(newNode);
            newNode = NULL;
        }
    }

    return newNode;
}

Indeed, there are many aspects of this function that as a beginner I find perplexing:

  1. Why is the DLLIST pointer returned this way, rather than being passed as a reference pointer and modified? If you did this, you could free up the return value for use as a status byte that reported function success/failure.
  2. Why is the second argument not tested to ensure that the pointer is not NULL? Surely passing a NULL pointer to memcpy would be a very bad thing?

Furthermore, is it common practice to force ALL functions within a module to use the same calling convention? For example, should all functions within the doubly-linked list module from which the function above is taken conform to status byte = function(argument list)? In addition, is it common practice to validate ALL function arguments whose value may be incorrect? For example, check all pointer values to ensure they are not null?

Improve this question

3 Answers 3

Reset to default
2

It's all rather subjective, I have to say.

1) Why is the DLLIST pointer returned this way, rather than being passed as a reference pointer and modified? If you did this, you could free up the return value for use as a status byte that reported function success/failure.

This is somewhat a matter of taste. I prefer to pass a pointer to the function so that the caller manages the memory, although returning a dynamically allocated pointer is also valid. In some cases, it's better and simpler, depending on the API architecture. Either way, the return value can be used for success/failure checks, as returning a NULL pointer would indicate failure.

2) Why is the second argument not tested to ensure that the pointer is not NULL? Surely passing a NULL pointer to memcpy be a very bad thing?

That is a mentality a lot of C programmers have (those I know, anyway) , which also reflects how the language is built. In most C libraries, each function has a contract, and not respecting it invokes undefined behavior. I rarely bother checking for NULL arguments being passed to functions. When I do, it's because the NULL pointer changes the behavior of the function, not because I'm making sure the caller respected the contract. Some may argue that it's better performance-wise and/or size-wise because it saves a few instructions here and there (especially true on limited hardware, for which C is known for), I mostly do it not to clutter my code with null-checks all over the place. You can do either, as long as it's properly documented.

Furthermore, is it common practice to force ALL functions within a module to use the same calling convention?

I have to agree with that. I can't speak for others, but I believe generally accepted as good practice. Switching calling conventions between functions in the same API can confuse programmers and have them waste time checking the documentation over and over.

In addition, is it common practice to validate ALL function arguments whose value may be incorrect?

It is common, but the opposite is probably quite as common (refer to #2 above).

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

Even then, would you prefer status = dlGetLength(list, &len) or len = dlGetLength(list)? I prefer the guideline that says that an interface should be predictable (which allows flexibility over rigid consistency).
@OmriBarel: I don't believe in predictable interfaces. It is subjective, because it depends on the previous experience of the programmer using similar interfaces. What I believe in, however, is proper documentation no matter the chosen style.
1

Why is the DLLIST pointer returned this way, rather than being passed as a reference pointer and modified? If you did this, you could free up the return value for use as a status byte that reported function success/failure.

Returning the result through a parameter passed-in from the outside is a rather inelegant and cumbersome practice, which should only be used when there's no other way. It requires the caller to define a recipient object, which is already bad by itself. But on top of that the need for additional declarations precludes the use of the function in pure expressions.

Most of the time, if you can implement your functionality through a pure function, it should be implemented through a pure function. I.e. function parameters are for input, function return value is for output. This is exactly what you see in this case.

Why is the second argument not tested to ensure that the pointer is not NULL? Surely passing a NULL pointer to memcpy would be a very bad thing?

Well, no argument, it is always a good idea to perform such tests. However, the exact method of testing can vary, depending on the "level" of the function in the program hierarchy (from "low level" to "high level"). Should it be a debug-only assertion? Should it be a permanent assertion (i.e. a controlled abort, with error log and "call me maybe" message)? Should it be a run-time test that implies some meaningful recovery strategy? There's no "one true rule" for answering this questions. It is a matter of code design.

Is there a meaningful fail-safe strategy for this function that should be executed in situations when the test fails? If not, then a run-time test in a low level function would make no sense whatsoever. It would only clutter the code with noise and waste performance. If this is indeed a "low level" function, then a run-time test with if is not really appropriate here. What is more appropriate is a debug-only assertion assert(object != NULL && size > 0)

Note also that the detail that makes the object != NULL test appropriate in dlCreate specifically is that the object value is passed to the library function memcpy, which is known to produce undefined behavior in case of null pointer input. If instead of memcpy the code used some proprietary my_copy_data function, then the proper place to assert the validity of object would be the innards of my_copy_data. The dlCreate function itself does not really care about the validity of object value.

Improve this answer

1 Comment

One of my numerous unasked questions concerned where abouts in a code hierarchy assert versus if testing should take place. Your lucid answer covered this ground, as well as the question I actually asked, so thank you.
1

Answering question 1: There are three common conventions for return values, and you can pick the one that best fits your application:

  1. Return status values (int). Typically negative values indicate error, >=0 success. For example, the POSIX "open()" function. If you need to pass some object, you pass a pointer to it (for "IN" parameters, or a pointer to a pointer to it (for "OUT" parameters)
  2. For "object-oriented" functions, particularly for functions that allocate something, return a pointer to the object (as in the doubly-linked-list example you give). Return NULL for failure. Think POSIX "fopen()" as an example.
  3. Absolute value functions. These are ones where the value does not indicate success or failure, but are the actual result of some computation (think "sin()").

You can mix and match these styles to your own taste. Many libraries have a mixture. Try not to make the user of the API have to do contortions to use it!

For example, using your code above, if you changed the API to

int dlCreate(uint16 tag, void *object, uint32 size, DLLIST **list)

Now a user has TWO ways of figuring out if the file was opened (look at the exit status, AND look at the filled in list pointer), and has to do a lot of extra work to figure out what is the right thiong to do for an error. So instead of writing

DLLIST *list;
if ((list = dlCreate(tag, object, sz) == NULL) {
  // handle out of memeory error
  ...

the user has to say

DLList *list = NULL // must initializein this case
if (dlCreate(tag, object, sz, &list) == ERROR || list == NULL)  {
  if (list != NULL) {
    free(list);
  }
  // and other error handling as above

That said, it is often good practice to use a consistent style for a library.

Answering Question 2: This is sloppy programming, and you are right; a good programmer should never trust the caller to "do the right thing". Always add code to validate arguments. If you passed a bad pointer here, you would get a crash!

Improve this answer

1 Comment

Looking at it from the position of the user of the API is great way to determine which return value paradigm is the best - thank you for the suggestion.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.