2

I once saw a snip of code as below,

/** Starts a synchronized block
*
*   This macro starts a block synchronized on its argument x
*   Note that the synchronized block defines a scope (i.e. { })
*   All variables declared in it will live inside this block only
*/
#define SYNCHRONIZE_ON(x) { \
                const abcd::LockBase & __lock = \
                                abcd::MakeLock(x); __lock;

/** Ends a synchronized block */
#define END_SYNCHRONIZE   }

The SYNCHRONIZE_ON and END_SYNCHRONIZE are used together to synchronize on an object. The macro SYNCHRONIZE_ON defines a variable ____lock in it's block.

The question here is: what is the sentence __lock; (after the abcd::MakeLock(x);) for? Notice that this sentence consist of only the variable name.

tbert
  • 2,089
  • 13
  • 14
Derui Si
  • 1,085
  • 1
  • 9
  • 13
  • 1
    It's statement, not sentence. – nhahtdh Jul 16 '12 at 07:04
  • Unless that macro is the implementation's, it's cutting into the reserved names with the 2 leading underscores. – chris Jul 16 '12 at 07:08
  • 7
    Most likely to suppress unused variable warnings. – Xeo Jul 16 '12 at 07:09
  • If I were you I would check the SYNCHRONIZE_ON() macro... it builds a reference (const abcd::LockBase __&__) to a temporary object. Thus either there is a typo, or MakeLock() returns a reference to something... that is not destructed. References have no desctructor they just vanish at the end of the block. And yes, the __lock, is just there to avoid a warning. But either this code has a typo, or it will dead-lock, or it does nothing... – armel Jul 16 '12 at 07:18
  • It would be interesting to see what `abcd::MakeLock()` returns; I don't see why `__lock` is a reference. – James Kanze Jul 16 '12 at 07:24
  • 1
    On some compilers it will generate a "statement has no effect" warning instead, so it is not a very good trick. – Bo Persson Jul 16 '12 at 07:26
  • @Xeo If `abcd::LockBase` has a non-trivial destructor, it's a very poor compiler which will warn about an unused variable. – James Kanze Jul 16 '12 at 07:35
  • @James: Know the [`ScopeGuard` trick](http://stackoverflow.com/q/4985800/500104)? I suspect something similar here. – Xeo Jul 16 '12 at 07:36
  • @JamesKanze: as the `__lock` variable itself is a reference no destructor is linked to that, so any decent compiler would still warn, and in this case probably with good reason. – KillianDS Jul 16 '12 at 07:54
  • I don't see RAII happening with this set of macros. Why to keep reference? – Ajay Jul 16 '12 at 08:08
  • abcd::MakeLock(x) will return a instance of the child class of LockBase. – Derui Si Jul 16 '12 at 08:10
  • @KillianDS I don't see why. If `abcd::MakeLock` returns a value, the temporary has the same lifetime in the two cases. (Of course, if `abcd::MakeLock` returns a reference, one has to wonder about the name, and possibly about the lifetime of what the reference refers to.) – James Kanze Jul 16 '12 at 08:15
  • @ageek2remember In which case, one has to wonder why the definition is a reference, and not simply a value. The semantics are exactly the same in the two cases. (One also has to wonder about the semantics of an RAII class which supports copy.) – James Kanze Jul 16 '12 at 08:17
  • @JamesKanze, see the definition of abcd::MakeLock, inline Lock MakeLock(X x) {return Lock(x)}, while Lock is the child class of LockBase. I believe it has to be a reference because that we need to call the desctructor of the child-class at the end of the block. – Derui Si Jul 16 '12 at 08:47
  • @ageek2remember They may use the base class just to "keep the door open" for future improvements (specialized locks per class type, named locks or something like that). In this way they can be sure that existing code won't be broken. – Adriano Repetti Jul 16 '12 at 09:50
  • 1
    @ageek2remember You're returning by value, so the class has to support copy. Which makes it rather tricky to implement a correct RAII. But whatever: the semantics of using a non-reference type return value to initialize a const reference or to initialize a value are, for all practical purposes, identical. All the use of the reference does is confuse the reader (and make one wonder if the author actually knows C++). – James Kanze Jul 16 '12 at 11:10

2 Answers2

9

Image you have this code:

SYNCHRONIZE_ON(myVariable)
   // Do stuff
END_SYNCHRONIZE

It'll be rewritten to:

{
    abcd::LockBase& __lock = abcd::MakeLock(myVariable);
    __lock;

    // Do stuff
}

Actually the __lock variable won't be used in your code, it's there only to dispose the critical section (if it's what it uses) when it'll go out of scope.

The problem with that code is that it'll generate tons of warning because __lock is declared but never used. That statement is there to prevent those warnings and it'll be optimized away by the compiler (because it has no side effects), this is what your compiler will actually perform:

{
    abcd::LockBase& __lock = abcd::MakeLock(myVariable);

    // Do stuff
}

EDIT
This code should suppress "unused variable" warning but it may not save from "expression has no effect" warning. pragmas to disable specific warning aren't portable at all and Internet is full of workarounds to avoid them, like this (it seems to be pretty portable across compilers).

{
    abcd::LockBase& __lock = abcd::MakeLock(myVariable);
    (void)__lock;

    // Do stuff
}

In this case a better solution may be to split declaration and lock acquisition, like this:

{
    abcd::LockBase& __lock = abcd::MakeLock(myVariable);
    __lock.Acquire();

    // Do stuff
}
Adriano Repetti
  • 65,416
  • 20
  • 137
  • 208
4

From this code it's very hard to guess what __lock is. Generally you should never name your stuff with names that start with __, because those are reserved for compiler developers.

In your case, the __lock can be either a macro (more likely), or a global variable (less likely).

You also have this line: const abcd::LockBase & __lock. Most likely this line declares a reference __lock to an object of a class LockBase.

So, here: const abcd::LockBase & __lock = abcd::MakeLock(x); we just execute a function call. However, __lock isn't used anywhere else within the block, so the compiler will give you a warning about "unused variable __lock". Therefore, the __lock; statement is probably added to suppress this warning. This is, to mention, a way of warning-suppression that Herb Sutter recommends in his book "C++ Coding Standards: 101 Rules, Guidelines, and Best Practices"

Sorry if my explanation is a bit messy, hope you'll get the general idea.

SingerOfTheFall
  • 29,228
  • 8
  • 68
  • 105