4
\$\begingroup\$

It seems like this code should be shorter, but with all the error checking it is long and hard to follow. This is for a simulation of vehicles crossing a bridge, and this part is dealing with mutexes. I tried extracting a function that is passed the mutex and locks/unlocks it and prints any errors, but when I did that it broke the code (it's been my experience on this site to not include any broken code so I won't elaborate on what I tried).

/*global variables*/
pthread_mutex_t Bridge = PTHREAD_MUTEX_INITIALIZER;
pthread_mutex_t okToOutput = PTHREAD_MUTEX_INITIALIZER;
/*...*/
/*this is the function that gets passed to pthread_create()*/
void* startSim(void *arg)
{

    Vehichle* inData = (Vehichle*)arg;
    if(usleep(inData->sleepT1))
        cerr << "Vehichle " << inData->licencePlate << " failed to sleep" << endl;
    if(pthread_mutex_lock(&okToOutput))
        cerr << "failed to lock output" << endl;
    cout << "Vehichle " << inData->licencePlate << " is ready to go " << dirToStr(inData->direction) << endl;       
    if(pthread_mutex_unlock(&okToOutput))
        cerr << "failed to unlock output" << endl;

    if(pthread_mutex_lock(&Bridge))
        cerr << "failed to lock bridge" << endl;
    if(pthread_mutex_lock(&okToOutput))
        cerr << "failed to lock output" << endl;
    cout << "Vehichle " << inData->licencePlate << " is ON the main bridge going " << dirToStr(inData->direction) << endl;
    if(pthread_mutex_unlock(&okToOutput))
        cerr << "failed to unlock output" << endl;
    if(usleep(inData->sleepT2))
        cerr << "Vehichle " << inData->licencePlate << " failed to sleep" << endl;
    if(pthread_mutex_lock(&okToOutput))
        cerr << "failed to lock output" << endl;
    cout << "Vehichle " << inData->licencePlate << " is OFF the main bridge after going " << dirToStr(inData->direction) << endl;
    if(pthread_mutex_unlock(&okToOutput))
        cerr << "failed to unlock output" << endl;
    if(pthread_mutex_unlock(&Bridge))
        cerr << "failed to lock bridge" << endl;

    return NULL;
}

My compiler doesn't properly support C++11 so no smart pointers etc.

\$\endgroup\$
  • \$\begingroup\$ There's not really much here to review other than the usual advice: don't use using namespace std; and put the lock/write/unlock of the okToOutput into a function. \$\endgroup\$ – Edward Oct 27 '14 at 1:54
  • \$\begingroup\$ @Edward "put the lock/write/unlock of the okToOutput into a function" I tried to do that but it broke the code. What would the function signature look like? I'm uncertain how I would pass the mutex. \$\endgroup\$ – Celeritas Oct 27 '14 at 2:12
  • 3
    \$\begingroup\$ "Vehicle" is misspelled. \$\endgroup\$ – 200_success Oct 27 '14 at 2:51
  • \$\begingroup\$ I am not familiar with threads but do you want to unlock something if you where not able to lock it? And you are not interested in the error number if an error occurs? \$\endgroup\$ – miracle173 Oct 27 '14 at 5:49
10
\$\begingroup\$

As mentioned in the comments, there's not much to review.

Don't abuse using namespace std

Putting using namespace std within your program is generally a bad habit that you'd do well to avoid.

Factor out common code

In this case, the code gets a lock, does something, and releases the lock. That could easily be isolated to a common function:

class Locker
{
     pthread_mutex_t&   mutex;
     bool               ok;
   public:
     operator bool() const {return ok;}

     Locker(pthread_mutex_t& mutex)
         : mutex(mutex)
         , ok(true)
     {
         if(pthread_mutex_lock(&mutex) != 0) {
             cerr << "failed to lock output" << endl;
             ok = false;
         }
     }
     ~Locker()
     {
         if (ok) {
             if(pthread_mutex_unlock(&mutex) != 0) {
                 cerr << "failed to unlock output" << endl;
             }
         }
     }
};          
void report(Vehicle *v, const char *msg)
{
    Locker    lock(okToOutput);
    if (lock) {
        cout << "Vehicle " << v->licencePlate << msg << dirToStr(v->direction) << endl;       
    }
}

To do this correctly, a failure to lock should exit the routine and not attempt to send output data.

Avoid C-style casts

Instead of (Vehicle *)arg use static_cast<Vehicle *>(arg). It makes much more clear to humans reading your code what you're doing.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ In the bit about static_cast, it might also be worth mentioning that static_cast is a much more conservative cast than a C-style cast. It's not really meaningful here since a void* can be implicitly converted into any other pointer, but eh... \$\endgroup\$ – Corbin Oct 27 '14 at 3:14
  • \$\begingroup\$ Instead of (Vehicle *)arg use static_cast<Vehicle *>(arg). It makes much more clear to humans reading your code what you're doing. Funnily, the first one is more clear to me upon first sight :) But then again, C++ is not my main language... \$\endgroup\$ – Juha Untinen Oct 27 '14 at 12:19
  • \$\begingroup\$ Made it exception safe. Remember you can set streams to throw an exception rather than change the error state flags. If the stream is already in an error state and the exception state is set trying to print on it will throw an exception. \$\endgroup\$ – Martin York Oct 27 '14 at 18:12
  • \$\begingroup\$ @LokiAstari: good point; the updated code looks good. \$\endgroup\$ – Edward Oct 27 '14 at 18:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.