Generic Stack implementation in C-style C++

Question

I implemented generic stack in C. Seems to work fine. It doesn't work for strings, though.

It was compiled using Visual C++.

Implementation:

#include <stdlib.h>
#include <stdio.h>
#include <iostream>
#include <string>
struct node{
 void* value;
 node* next;
};
struct stack{
 node* first;
 int elemSize;
 int length;
};
stack* StackNew(int elemSize)
{
 stack* s = (stack*)malloc(sizeof(stack));
 s->elemSize = elemSize;
 s->length = 0;
 return s;
}
void Push(stack* s, void* value)
{
 node* oldFirst = s->first;
 s->first = (node*)malloc(sizeof(node));
 s->first->value = malloc(s->elemSize);
 memcpy(s->first->value, value, s->elemSize);
 s->first->next = oldFirst;
 s->length++;
}
void* Pop(stack* s)
{
 void* value = malloc(s->elemSize);
 memcpy(value, s->first->value, s->elemSize);
 node* oldFirst = s->first;
 s->first = s->first->next;
 free(oldFirst);
 s->length--;
 return value;
}
void Destory(stack* s)
{
 while (s->length > 0) Pop(s);
 free(s);
}

Test:

struct point{
 int x, y;
};
int main()
{
 stack* s = StackNew(sizeof(point));
 for (int i = 0; i < 100; i++){
 point pt;
 pt.x = i;
 pt.y = i * i;
 Push(s, &pt);
 }
 while (s->length > 0) {
 point pt = *(point*)Pop(s);
 printf("(%d,%d)\n", pt.x, pt.y);
 }
 Destory(s);
 return 0;
}

Output:

(99,9801)
(98,9604)
(97,9409)
(96,9216)
(95,9025)
(94,8836)
(93,8649)
(92,8464)
(91,8281)
(90,8100)
(89,7921)
(88,7744)
(87,7569)
(86,7396)
...

as expected.

Please tell me what I did wrong and how I can improve things.

• Quaxton Hale: If this is C, why are you including iostream?
• Rezo Megrelidze: @EngieOP I wrote it in C++ but i'm using the C subset of C++.
• Quaxton Hale: Okay, just wondering. I remember there being a Meta post about C/C++ tag mixing.
• 200_success: If it compiles as C++, but would fail to compile as C, then it's still C++ code and should be tagged as such. (Someone will probably say "Why don't you just go all the way with C++?" in an answer, but that's just how it has to be.)
• 200_success: Alternatively, just remove #include <iostream> and retag as c.

Answer 1

• Naming seems inconsistent.

  Consider prefixing all exported names with Stack, e.g. StackPush, StackPop, StackDestroy (Destory is definitely a typo).

  The entity named first in a stack context is traditionally referred to as top.

• Memory management is overengineered.

  Every time a value is pushed memory is allocated. That's justifiable for ownership reasons (make sure to document it). However every time a value is popped memory is allocated again. This is very strange. A simple

  return s->first->value;
  

  would suffice.

  Along the same line Destroy doesn't free memory (re)allocated for individual stack entries. Consider

  while(s->length > 0)
   free(Pop(s))
  

• You've mention the problem with strings; I believe it is stemmed from elemSize being a property of the stack itself. Making it a property of an element would solve the problem.

• As mentioned in the comments, the C code shall not include C++ headers.

• Rezo Megrelidze: "Making it a property of an element would solve the problem." It does solve the problem. I also had to to rewrite StackPush(Stack* s,void* value) to StackPush(Stack* s,void* value,int elemSize).