Hacker News Reader

Comment by tylertreat | Hacker News Reader

Thanks, 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 :)

View on HN

What 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 :)

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

You 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;
     ...
  };

Got it, that makes more sense.

You 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;

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

Reply on news.ycombinator.com