Does returning `INT_MIN` as an error code have unwanted consequences?

Question

I have a simple push/pop implementation in my program:

void Push(int *stack, int *top, int item)
{
  if (*top == SIZE - 1) {
    printf("Overflow\n");
    return;
  } else if (item == INT_MIN) {
    printf("Item size out of bounds\n");
    return;
  }
  stack[++*top] = item;
}

int Pop(int *stack, int *top)
{
  if (*top == -1) {
    printf("Empty\n");
    return INT_MIN;
  }
  return stack[*top--];
}

In the Pop() function, I need to return some kind of error code when the stack is empty but since all of the int values are valid returns: Can I resort to reserving INT_MIN for this? Is this a 'good practice', have unwanted consequences, etc?

It precludes you from inserting INT_MIN into your data structure. sizeof int is platform specific so the particular value may change. If you wanted to store longs (so replacing all int with long) then INT_MIN would probably be wrong as it's a "random" value in the middle so you would need to change the constant, too. It's also not just INT_MIN as you may want a code for each distinct error type. For instance overflow vs out of bounds. My advise would be to not mix your data and error domains (use different variables and/or return value). — Allan Wind, Jan 22 '23 at 06:22
@AllanWind what is an 'out parameter' in C? Also I didn't fully fully understand your alternative options — 3rdgrade-dropout, Jan 22 '23 at 06:28
Pointer to the variable you want to change (like your stack or top variables). `enum error Pop(int *stack, int *top, *value)` and then you define your errors with an enum which will much better document each type. You want a specific error if the client can do anything useful for it. Say, FULL, might be an error and client could now pop and element then try push the value again. Btw, it's a good idea to use a namespace prefix for all symbols you export, for instance `stack_pop()`, `stack_push()`, STACK_FULL etc. — Allan Wind, Jan 22 '23 at 06:29
OT: Why worry about the error condition in `pop()` when `push()` doesn't return any error code? — Fe2O3, Jan 22 '23 at 06:33
@Chqrlie has written some solid code below (that you may wish to "accept" as a useful answer.) Just returning to my keyboard, having thought to recommend "Look for analogous operations!" Consider `fwrite()` and `fread()` that store/retrieve data... The data is referenced as a parameter, leaving the function return code for use to indicate success or failure. — Fe2O3, Jan 22 '23 at 07:14

score 1 · Answer 1 · answered Jan 22 '23 at 07:10

Retuning a magic value that should actually be part of the range of possible values is a bad design choice. It artificially reduces the range of values, makes Push less efficient, and it less easy to test at call sites.

A better approach is for Pop() to return a success indicator and update both the stack index and the popped value via pointers. The same applies to Push(): returning a success indicator allows for the caller to test and decide on the best action.

Also note a major problem in your implementation: *top++ will not increment the value of *top, but the value of the pointer top. You must write (*top)++ for proper operation.

enum {
    NO_ERROR = 0,
    STACK_OVERFLOW = 1,
    STACK_UNDERFLOW = 2,
};

int Push(int *stack, int *top, int item) {
    if (*top >= SIZE - 1) {
        printf("Overflow\n");
        return STACK_OVERFLOW;
    } else {
        stack[++*top] = item;
        return NO_ERROR;
    }
}

int Pop(int *stack, int *top, int *value) {
    if (*top <= -1) {
        printf("Empty\n");
        // whether to update *value in this case is a design decision
        return STACK_UNDERFLOW;
    } else {
        *value = stack[(*top)--];
        return NO_ERROR;
    }
}

Allan Wind · Answer 2 · 2023-01-22T06:59:31.510

It's bad design to have overlapping data and error domains.

It is brittle to change, say, you want to add a new error code, now you have audit all calls to your function to ensure that other value is being used for something important. You need to guard against your magic values being inserted, otherwise you cannot tell an error from a pop() of that value. As this implements an abstract data type (ADT) you may find you want versions for other types, say, long, or double, and here INT_MIN makes little sense, so you would have to adapt it for each type.

You need to document that INT_MIN means error, and how you can't use it as a value to push(). You would want to use a better constant name. It will not be obvious to someone reading your code that return INT_MAX means there was an error, so you ought to write comments for your future self.

The size of int is platform specific. For example, if you log error code INT_MAX, it might be -2^31 on system and -2^63 on another.

Ideally, you want a distinct error code for each error beneficial for client, which is probably more than 1 error code. A library, like this, is much more reusable if you leave the UI to the calling code (have caller printf() error messages).

There are many valid design options, btw, and you may benefit for reviewing the framework I outlined in my answer to Error handling in C code.

Does returning `INT_MIN` as an error code have unwanted consequences?

2 Answers2