3 ms·
Good stuff, thanks for sharing. Random comments based on a quick glance through the code if I may. chan_t* chan_init(int capacity); should be "size_t capaci
by eps 12y ago
Good stuff, thanks for sharing. Random comments based on a quick glance through the code if I may.
chan_t* chan_init(int capacity);
should be "size_t capacity", since negative capacity values are invalid. This also eliminated the need for the first check in the function.
chan->buffered
is duplicate in meaning to chan->queue being non-NULL.
Why chan's mutexes and pcond are malloc'ed and not included in chan's struct? They are also not freed when channel is disposed, unless I'm missing something. Mutexes are also not destroyed when buffered chan is disposed, but they are created for it.
(Edit)
Also, my biggest nitpick would be that chan_t structure should really not be in chan.h header since it's meant to be transparent to the app and the API operates exclusively with pointers to chan_t. Then, if it is made private (read - internal to the implementation), then you can divorce buffered and unbuffered channels into two separate structs, allocate required one and then multiplex between them in calls that treat buffered and unbuffered channels differently. I.e.
struct chan_t
{
// Shared properties
pthread_mutex_t m_mu;
pthread_cond_t m_cond;
int closed;
// Type
int buffered;
};
struct chan_unbuffered_t
{
struct chan_t base;
pthread_mutex_t r_mu;
pthread_mutex_t w_mu;
int readers;
blocking_pipe_t* pipe;
};
...
or better yet, just include a handful of function pointers into chan_t for each of the API methods that need more than the shared part of chan_t struct, initialize them on allocation and put type-specific code in these functions. It will make for a more compact, concise and localized code.
(Edit 2)
Here's what I mean with function pointers - https://gist.github.com/anonymous/5f97d8db71776b188820 https://gist.github.com/anonymous/5f97d8db71776b188820 - basically poor man's inheritance and virtual methods :)
- tylertreat 12y agoThanks, I'm not much of a C programmer, so I appreciate the feedback. I'm not sure what you're referring to with the mutexes and pcond not being on the struct because they are. Can you clarify? You are right with the disposal though, I need to clean that stuff up :)
- eps 12y agoYou don't need to allocate pthread_mutex_t instances on heap, you can include them into the struct directly - struct foo { pthread_mutex_t bar; ... };
- tylertreat 12y agoGot it, that makes more sense.
- sdab 12y agoI'm more of a c++ programmer, but I think i can chime in on the mutex/pcond being "part" of the struct. Essentially, OP means that why arent they on chan's stack? In the struct you have pointers to mutex and pcond which means you have to malloc them. OP is suggesting they not be pointers.
- millstone 12y agoWhat eps means is that, currently you have a pointer to a mutex in your struct: struct chan_t { pthread_mutex_t* m_mu;... } And initialize it like so: pthread_mutex_t* mu = (pthread_mutex_t*) malloc(sizeof(pthread_mutex_t)); pthread_mutex_init(mu, NULL); chan->m_mu = mu; eps's suggestion is to put the mutex directly in the struct: struct chan_t { pthread_mutex_t m_mu;... } and initialize it by passing its address to pthread_mutex_init: pthread_mutex_init(&chan->m_mu, NULL); chan->m_mu = mu; that saves some mallocs and dereferences. If you are new to C, the notion of taking the address of fields in a struct may be unnerving, but you get used to it :)
- pmahoney 12y agoThe struct is currently storing a pointer to the mutex (which was allocated with malloc). The call to pthread_mutex_destroy does not free that memory. Valgrind is your friend :) http://valgrind.org/docs/manual/quick-start.html#quick-start.prepare http://valgrind.org/docs/manual/quick-start.html#quick-start...
- clarry 12y agoYou shouldn't allocate heap space for a pointer in blocking_pipe_read. Also, it looks like you're leaking it in the else branch. Just allocate a pointer on the stack, read into it, and then assign the value into the provided space: void *msg_ptr; read(..., &msg_ptr, sizeof msg_ptr); *data = msg_ptr;
- DSMan195276 12y agoIf he's gonna do it that way (Which I agree with and also recommend) he could probably use the container_of macro (You can implement it without gcc extensions - it has slightly less type checks however). Using the container_of he can safely turn the chan_t into a chan_unbuffered_t or chan_buffered_t or etc. assuming he does the proper checks beforehand to make sure it's actually inside of that type. Also worth noting, he really shouldn't be using '_t' since it's reserved by the standard.
- yew 12y agoThe container_of macro is intended for cases where contained objects are at some (possibly compile-time variable) offset into a structure. Eliding type-checking, it reduces to the simple cast in this particular case. Not to say that it wouldn't work, just that it isn't really necessary - casting a pointer to a structure to a pointer to its first member (and back) is perfectly portable. Perhaps even idiomatic. (Though the casts in that gist seem to be backwards?)
- DSMan195276 12y agoThat's exactly what we have here though, the 'base' chan_t structure is contained inside of the chan_unbuffered_t structure. And I disagree with your second point, I consider casting structures directly like that to be bad practice. It's ugly, error prone, and not guaranteed to work by the standard since struct ordering isn't guaranteed (IE. The standard doesn't say that 'base' will be at the beginning of the struct.). That said, it is 'portable' since in general no compilers reorder the struct members, but container_of is guaranteed to work, works for multiple members in a struct exactly the same way, and will reduce to a simple cast when your offsetof is zero so there isn't any performance penalty in situations like this. IMO there's no reason not to use container_of in this case.
- yew 12y agoThe standard permits padding structures, but not reordering their members. That's why container_of is necessary (to avoid constantly-updated manual offset calculations or writing redundant code). Any implementation that reorders structure members is non-compliant (and likely incompatible with much existing code). To quote the relevant section (C99 6.7.2.1.13, same wording present in C89): Within a structure object, the non-bit-field members and the units in which bit-fields reside have addresses that increase in the order in which they are declared. A pointer to a structure object, suitably converted, points to its initial member (or if that member is a bit-field, then to the unit in which it resides), and vice versa. There may be unnamed padding within a structure object, but not at its beginning. I'll repeat that I consider direct casts to be somewhat idiomatic. Macros are more traditional for offset members.