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\$
4
  • \$\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, 2014 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, 2014 at 2:12
  • 3
    \$\begingroup\$ "Vehicle" is misspelled. \$\endgroup\$ Oct 27, 2014 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, 2014 at 5:49

1 Answer 1

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.

\$\endgroup\$
4
  • \$\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, 2014 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\$ Oct 27, 2014 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\$ Oct 27, 2014 at 18:12
  • \$\begingroup\$ @LokiAstari: good point; the updated code looks good. \$\endgroup\$
    – Edward
    Oct 27, 2014 at 18:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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