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:

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 having it there directly.
(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.

Getting to the details now:

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. You might want to add a method to peek at the top:

    StackElement stack_peek(Stack s);
   

4. Perhaps also a method to replace the top:

    StackElement stack_replace(Stack s, StackElement v);
   

Sourcefile:

3. 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);
   

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

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

6. 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.

7. 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.

8. 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.