HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Coroutines in C

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
coroutinesstackoverflowprogramming

Problem

Please have a look at this little coroutines library ccoro: http://sam.nipl.net/code/ccoro

I'd appreciate a general code and style review, and your kind comments!

ccoro.h

/*
 * ccoro - Coroutines in C
 * Sam Watkins, 2009
 * this code is public domain
 *
 * ccoro uses setjmp and longjmp to achieve coroutines in plain C.
 */

#ifndef CCORO_H
#define CCORO_H 1
#ifdef __cplusplus
extern "C" {
#endif

#include 

#if defined (__GNUC__)
#define noret void __attribute__((noreturn))
#else
#define noret void
#endif

extern int coro_pad;
enum { coro_code_done = -1, coro_code_alloc = -2, coro_code_dead = -3 };

typedef struct coro coro;

typedef void (*coro_func)(coro *caller);

struct coro
{
    coro *next;
    coro *prev;
    jmp_buf j;
};

coro *new_coro(coro_func f);
int yield(coro **c);
int yield_val(coro **c, int val);

#ifdef __cplusplus
}
#endif
#endif


ccoro.c

```
// 2>/dev/null; exec cc -c -Wall -Wextra -O2 ccoro.c ; exit $? # /

/*
* ccoro - Coroutines in C
* Sam Watkins, 2009
* this code is public domain
*
* ccoro uses setjmp and longjmp to achieve coroutines in plain C,
* non-preemptive threading. It works by allocating each thread some stack
* space on the normal stack. It makes sure that each thread has enough space
* to run using a padding variable, of size 8k by default, which is inserted by
* a function that starts the thread. Simple, huh? It uses some fairly simple
* trickery to create new threads that don't overlap with the other threads.
*
* I don't claim that this code is legal by the book, but it seems to work
* with at gcc, tcc, tendra and lcc on Linux i386 and x86_64, and with mingw
* gcc-4.4 and Visual C++ 98 Express on Windows.
*
* As is, this requires C99. To run with C89, use an enum for coro_pad.
*
* An earlier version was reported not to work with tendra on netbsd.
* I have fixed bugs since then so maybe it works now.
*
* An earlier version did not work with -O2 on some systems, it seems to be
* work

Solution

ccoro.h

#define CCORO_H 1


Random note, but you don't need to define that to 1, a simple #define CCORO_H would work fine.

enum { coro_code_done = -1, coro_code_alloc = -2, coro_code_dead = -3 };


I'd make it a little clearer that these are constants. Due to C's general lack of typing or other constructs to make code self-documenting, prefixing constants with k or using ALL_CAPS is helpful.

ccoro.c

static noret new_coro_2(coro_func f, coro *caller);
static noret new_coro_3(coro_func f, coro *caller);
static int yield_val_2(coro *c, int val);


I know you mentioned this already, but these function names are meaningless and confusing. Make them more meaningful to make the code clearer.

int coro_pad = 8192;


What does this do? Add a comment to document this constant's purpose.

Also, coro_pad is a constant, right? Why not use const to declare it as such?

if (v == 0)                        /* yield */
    longjmp(c->j, val);
else if (v == coro_code_alloc) {   /* new - this is top */
    coro *caller = current_coro;
    current_coro = me;
    new_coro_2(new_coro_f, caller);
    /* cannot return */
}


Braces, please! If you're going to have an else clause with braces, put braces around the if clause, too.

noret new_coro_3(coro_func f, coro *caller)
{ ... }


Honestly, I have no idea what this function is doing. Changing the name would help, as mentioned above, but adding some inline comments would be nice, too!

Code Snippets

#define CCORO_H 1
enum { coro_code_done = -1, coro_code_alloc = -2, coro_code_dead = -3 };
static noret new_coro_2(coro_func f, coro *caller);
static noret new_coro_3(coro_func f, coro *caller);
static int yield_val_2(coro *c, int val);
int coro_pad = 8192;
if (v == 0)                        /* yield */
    longjmp(c->j, val);
else if (v == coro_code_alloc) {   /* new - this is top */
    coro *caller = current_coro;
    current_coro = me;
    new_coro_2(new_coro_f, caller);
    /* cannot return */
}

Context

StackExchange Code Review Q#79415, answer score: 8

Revisions (0)

No revisions yet.