Skip to main content
Code Review

Return to Answer

added 329 characters in body
Source Link
chux
chux
  • 36.5k
  • 2
  • 43
  • 97

Since code is passing 2 pointers of a specific type

Printing that datadatum

AsDown casting to int can lose information. As a minmax_stack function, better to avoid losingshow all information by down casting to int.

Use of NULL as datum

Code uses return value of NULL to indicate an error, yet a datum == NULL is not necessarily wrong in a general sense and so with a return value of NULL, it is ambiguous if that is an error or datum? To support datum == NULL, the architecture of this code may need significant re-write. Is this important? - Depends on coding goals. For a general purpose function set, I say yes.

Since code is passing 2 pointers of a specific type

Printing that data

As a minmax_stack function, better to avoid losing information by down casting to int.

Printing datum

Down casting to int can lose information. As a minmax_stack function, better to show all information.

Use of NULL as datum

Code uses return value of NULL to indicate an error, yet a datum == NULL is not necessarily wrong in a general sense and so with a return value of NULL, it is ambiguous if that is an error or datum? To support datum == NULL, the architecture of this code may need significant re-write. Is this important? - Depends on coding goals. For a general purpose function set, I say yes.

added 329 characters in body
Source Link
chux
chux
  • 36.5k
  • 2
  • 43
  • 97

preventPrinting that data

As a minmax_stack function, better to avoid losing information by down casting to int.

// printf("%d", (int) current_node->datum);
printf("%p", current_node->datum);
// or cast to widest type through `uintptr_t`
printf("%ju", (uintmax_t) (uintptr_t) current_node->datum);

Prevent buffer overrun

tolerateTolerate stack == NULL?

prevent buffer overrun

tolerate stack == NULL?

Printing that data

As a minmax_stack function, better to avoid losing information by down casting to int.

// printf("%d", (int) current_node->datum);
printf("%p", current_node->datum);
// or cast to widest type through `uintptr_t`
printf("%ju", (uintmax_t) (uintptr_t) current_node->datum);

Prevent buffer overrun

Tolerate stack == NULL?

Source Link
chux
chux
  • 36.5k
  • 2
  • 43
  • 97

Leak

When a node is popped, the node is not free'd.

compare()

Avoid int overflow which is undefined behavior (UB). Use the idiom (a>b)-(a<b). It is recognized by a number of compilers to make lean code and does not overflow.

static int compare(void* a, void* b) {
    // return (int)((int) a - (int) b);
    return ((int) a > (int) b) - ((int) a < (int) b);
}

Note that qsort() expects int (*compar)(const void *, const void *) and so follow that const signature. Adjust minmax_stack and functions accordingly.

Casting a void * to int may lose information. If conversion to an integer type is truly needed, consider the optional type intptr_t or better yet uintptr_t.

the property that any valid pointer to void can be converted to this type, then converted back to pointer to void, and the result will compare equal to the original pointer C11dr §7.20.1.4 1

static int compare(const void* a, const void* b) {
    uintptr_t ua = (uintptr_t) a;
    uintptr_t ub = (uintptr_t) b;
    return (ua > ub) - (ua < ub);
}

Since code is passing 2 pointers of a specific type

prevent buffer overrun

Although only test code, limit user input.

char command[100];
...
    // scanf("%s", command);
    scanf("%99s", command);

tolerate stack == NULL?

Should functions tolerate stack == NULL - IMO yes, but a design decision.

free(void *ptr) is tolerant of free(NULL), suggest the same for minmax_stack_free().

void minmax_stack_free(minmax_stack* stack) {
  if (stack == NULL) return;
  ...

Alloc size

Good use of sizing by the referenced type, rather than the size of the explicit type! Could add allocation success test.

//                                   size of referenced type 
minmax_stack_node* new_node = malloc(sizeof(*new_node));
if (new_node == NULL) TBD_Handle_Error();

Minor style implication: sizeof(*new_node) --> sizeof *new_node.

Variable declaration

A style issue, yet I find declaring variables when there are needed better as well as bracketed if()

// minmax_stack_node* node;
// void* return_value;
// if (stack->size == 0) return NULL;

if (stack->size == 0) { 
  return NULL;
}
minmax_stack_node* node = stack->top_node;
void* return_value = node->datum;