Return to Revisions

No, your implementation is broken:

You forgot to check whether allocation succeeded.

Reviewing your code, you are going way overboard following the "small methods"-mantra in your library-code:

While the Single responsibility principle means every function should do one thing, and only one thing, the size of each task should be properly chosen.

Too little, and the function isn't a useful abstraction, using, writing and remembering it takes more effort than it saves.
(The performance-cost may be negligible (or even non-existent), if it is inlined. Consider marking it static to promote inlining and avoid exporting.)

Too much, and reusability and composability suffer.

Header:

1. I would avoid a typedef for the element-type. What does it buy you?

2. Using an opaque pointer is a good idea allowing you to replace the implementation with something more performant later. So, well done there.

3. Consider leaving the parameter-name out of the declaration when it doesn't make it more descriptive.

4. You should add a variation on stack_push which signals success, so a caller can opt in to handling failure gracefully:

    bool stack_tryPush(Stack, StackElement);
   

5. Some more useful additions:

    StackElement stack_peek(Stack);
    StackElement stack_replace(Stack, StackElement);
   

Sourcefile:

6. You should re-order your includes: Always include the header you are implementing first, then all other headers, sorted for consistency (use the IDE's sorting-macro).
   That way, you will know that your header is self-sufficient and you didn't forget a crucial dependency.

7. There's abort() for abnormal termination. Consider writing a helper-function if you need to output an error-message and abort in multiple places:

    static void die(const char* message);
   

8. All your internal functions should not have external linkage, for performance and correctness reasons: Mark them inline.

9. Putting next first is more common, and possibly slightly more efficient. Also, for a single value value is more idiomatic than data.

10. Is there any reason why your typedef-name and struct-tag aren't quite equal? That's ittitating.

11. node_create is a waste: It's literally more work to read, write, and use it than leaving the single use inline.
    As a rule of thumb, extracting 2 lines into their own function is a net loss of readability. Exceptons are often-used pieces of code.

    Actually, that can be generalized to all your internal functions, with the possible exception of node_delete, which is marginal.

12. There are two kinds of precondition-checks:

    1. Sanity checks, verifying that the programs logic is sane. These are normally disabled in a release build for performance reasons, and use assert().
    2. Tests of untrusted input. How far you can/should/must trust an API's user must be decided in each case. It's your task as API-designer to decide how much you should penalize your users to catch egregious abuse even in release-mode, and whether there's also a debug-mode.
       But there's no excuse for making your internal helper-functions re-assert preconditions even in release-mode.

13. As you are using C99 (see <stdbool.h>), you can use compound-literals to set all members of a struct-type comfortably even after initialization.

     void stack_push(StackCDT *s, StackElement value)
     {
         Node *new = malloc(sizeof *new);
         *new = (Node){s->top, value};
         if(!new) die("Failed to allocate a new node.");
         s->top = new;
     }
    

Testprogram:

14. return 0; is implicit in main since C99.