Extra brace brackets in C++ code

Question

Sometimes you run into code that has extra brace brackets, that have nothing to do with scope, only are for readability and avoiding mistakes.

For example:

GetMutexLock( handle ) ; 
{
  // brace brackets "scope" the lock,
  // must close block / remember
  // to release the handle.
  // similar to C#'s lock construct
}
ReleaseMutexLock( handle ) ;

Other places I have seen it are:

glBegin( GL_TRIANGLES ) ;
{
  glVertex3d( .. ) ;
  glVertex3d( .. ) ;
  glVertex3d( .. ) ;
} // must remember to glEnd!
glEnd() ; 

This introduces a compiler error if the mutex isn't freed (assuming you remember both the } and the Release() call).

The question is:

• 1 Is this a bad practice? Why?
• 2 If it is not a bad practice, will it possibly change how the code is compiled? Possibly make it slower?

Answer 1

It's not bad practice. It does not make anything slower; it's just a way of structuring the code.

Getting the compiler to do error-checking & enforcing for you is always a good thing!

Answer 2

Anything that improves readablity IMHO is good practice. If adding braces help with the readability, then go for it!

Adding additional braces will not change how the code is compiled. It won't make the running of the program any slower.

Answer 3

Braces affect variable scope. As far as I know that is all they do.

Yes, this can affect how the program is compiled. Destructors will be called at the end of the block instead of waiting until the end of the function.

Often this is what you want to do. For example, your GetMutexLock and ReleaseMutexLock would be much better C++ code written like this:

struct MutexLocker {
  Handle handle;
  MutexLocker(handle) : handle(handle) { GetMutexLock(handle); }
  ~MutexLocker() { ReleaseMutexLock(handle); }    
};
...
{
  MutexLocker lock(handle);
  // brace brackets "scope" the lock,
  // must close block / remember
  // to release the handle.
  // similar to C#'s lock construct
}

Using this more C++ style, the lock is released automatically at the end of the block. It will be released in all circumstances, including exceptions, with the exceptions of setjmp/longjmp or a program crash or abort.

Answer 4

The braces themselves are fine, all they do is limit scope and you won't slow anything down. It can be seen as cleaner. (Always prefer clean code over fast code, if it's cleaner, don't worry about the speed until you profile.)

---

But with respect to resources it's bad practice because you've put yourself in a position to leak a resource. If anything in the block throws or returns, bang you're dead.

Use Scope-bound Resource Management (SBRM, also known as RAII), which limits a resource to a scope, by using the destructor:

class mutex_lock
{
public:
    mutex_lock(HANDLE pHandle) :
    mHandle(pHandle)
    {
        //acquire resource
        GetMutexLock(mHandle);
    }

    ~mutex_lock()
    {
        // release resource, bound to scope
        ReleaseMutexLock(mHandle);
    }

private:
    // resource
    HANDLE mHandle;

    // noncopyable
    mutex_lock(const mutex_lock&);
    mutex_lock& operator=(const mutex_lock&);
};

So you get:

{
  mutex_lock m(handle);
  // brace brackets "scope" the lock,
  // AUTOMATICALLY
}

Do this will all resources, it's cleaner and safer. If you are in a position to say "I need to release this resource", you've done it wrong; they should be handled automatically.

Answer 5

It will make no difference to the compiled code, apart from calling any destructors at the end of that block rather than the end of the surrounding block, unless the compiler is completely insane.

Personally, I would call it bad practice; the way to avoid the kind of mistakes you might make here is to use scoped resource management (sometimes called RAII), not to use error-prone typographical reminders. I would write the code as something like

{
    mutex::scoped_lock lock(mutex);
    // brace brackets *really* scope the lock
}   // scoped_lock destructor releases the lock

{
    gl_group gl(GL_TRIANGLES); // calls glBegin()
    gl.Vertex3d( .. );
    gl.Vertex3d( .. );
    gl.Vertex3d( .. );
} // gl_group destructor calls glEnd()

Answer 6

This is much more useful (IMHO) in C++ with object destructors; your examples are in C.

Imagine if you made a MutexLock class:

class MutexLock {
private:
    HANDLE handle;
public:
    MutexLock() : handle(0) {
        GetMutexLock(handle);
    }

    ~MutexLock() {
        ReleaseMutexLock(handle);
    }
}

Then you could scope that lock to just the code that needed it by providing an new scope with the braces:

{
    MutexLock mtx;  // Allocated on the stack in this new scope

    // Use shared resource
}
// When this scope exits the destructor on mtx is called and the stack is popped

Answer 7

If you're putting code into braces, you should probably break it out into its own method. If it's a single discreet unit, why not label it and break it out functionally? That will make it explicit what the block does, and people who later read the code won't have to figure out.

Answer 8

The specific placement of { ... } in your original example serves purely as formatting sugar by making it more obvious where a group of logically related statements begins and where it ends. As shown in your examples, it has not effect on the compiled code.

I don't know what you mean by "this introduces a compiler error if the mutex isn't freed". That's simply not true. Such use of { ... } cannot and will not introduce any compiler errors.

Whether it is a good practice is a matter of personal preference. It looks OK. Alternatively, you can use comments and/or indentation to indicate logical grouping of statements in the code, without any extra { ... }.

There are various scoping-based techniques out there, some of which have been illustrated by the other answers here, but what you have in your OP doesn't even remotely look anything like that. Once again, what you have in your OP (as shown) is purely a source formatting habit with superfluous { ... } that have no effect on the generated code.