CODEWITHSUNDEEP
c++scopec++17
mrks
mrks

Reputation: 8333

lock_guard initialization in if-initializer instead of simple scope

Is it correct that

if (std::lock_guard lk(m_state.mtx); true) {
  m_state.aborting = true;
}

and

{
  std::lock_guard lk(m_state.mtx)
  m_state.aborting = true;
}

are 100% identical?

Here is the background on my question: In Mastering the C++17 STL, the author gives a code example for a thread pool. It contains the following block (page 208):

~ThreadPool() {
  if (std::lock_guard lk(m_state.mtx); true) {  // <<--- Line of interest
    m_state.aborting = true;
  }

  m_cv.notify_all();
  for (std::thread& t : m_workers) { 
    t.join();
  }
}

The marked line uses the initializer part of an if-statement to create the lock_guard. This has been used in other questions in order to limit the scope of the lock_guard to the if-statement while also protecting the evaluation of the if-statement itself.

What confuses me in this example is that the if statement has no proper condition. According to the cppreference page on if,

If init-statement is used, the if statement is equivalent to

{
  init_statement
  if ( condition )
    statement-true
}

In this case, this would mean that if (std::lock_guard lk(m_state.mtx); true) {...} is equivalent to

{
  std::lock_guard lk(m_state.mtx);
  if (true) {
    m_state.aborting = true;
  }
}

Am I missing either a case where this is not 100% identical or a good reason why it has been written with an if-statement?

The same pattern is used in a different example on the next page, so I think that it was done on purpose and not a leftover from when there was an actual condition.

Upvotes: 3

Views: 535

Answers (2)

Quuxplusone
Quuxplusone

Reputation: 27155

I'm the author of Mastering the C++17 STL.

I'd already forgotten that construct, so there can't have been any important reason I used it. I believe Igor is correct: the choice would have been between

~ThreadPool() {
  if (std::lock_guard lk(m_state.mtx); true) {
    m_state.aborting = true;
  }
  m_cv.notify_all();
  for (std::thread& t : m_workers) {
    t.join();
  }
}

and something such as

~ThreadPool() {
  if (true) {
    std::lock_guard lk(m_state.mtx);
    m_state.aborting = true;
  }
  m_cv.notify_all();
  for (std::thread& t : m_workers) {
    t.join();
  }
}

which does the same thing (arguably clearer) but takes one extra line of code in the physical book.

Personally I don't like to write just { to open a block; I would always rather write if (true) {. This helps the reader understand that the if wasn't accidentally omitted (or merge-conflicted away); the programmer really intends the block to run always. If you cuddle all your braces all the time, as I (often) do, then it also maintains the invariant that you never have a { or } sitting alone on a line — for whatever that might be worth.

The other thing going on there is that it's specifically in the context of notifying on a condition variable. I definitely used to write

std::lock_guard lk(m_state.mtx);
m_state.aborting = true;
m_cv.notify_all();

with the notify happening inside the scope of the lock; over a period of years, I heard "you should release the lock before notifying" often enough that eventually I also started saying it (even though I still have no good intuition for why that should be the case). This was actually the subject of my fourth-ever SO question, five years before I wrote the book: C++11 std::condition_variable: can we pass our lock directly to the notified thread?

So I may have been distracted (underthinking) and/or self-conscious (overthinking) while writing this particular snippet, which might have led me to try out this particular weird construct.

The example on the next page is actually the condition-variable thing exactly:

void enqueue_task(UniqueFunction task) {
  if (std::lock_guard lk(m_state.mtx); true) {
    m_state.work_queue.push(std::move(task));
  }
  m_cv.notify_one();
}

Thread-safety-wise, this is exactly equivalent to

void enqueue_task(UniqueFunction task) {
  std::lock_guard lk(m_state.mtx);
  m_state.work_queue.push(std::move(task));
  m_cv.notify_one();
}

except that people kept telling me not to put the notify inside the lock. So the if (...; true) version grudgingly satisfies that popular wisdom while spending only one line of code instead of two.

It's even possible that I had originally notified under the lock, and then the book's one technical reviewer said "don't notify under the lock," and then I made this late-breaking change specifically to deal with that in the minimal number of lines. I don't remember whether that was the actual story, but it's quite plausible to me.

Upvotes: 1

bolov
bolov

Reputation: 75697

They are semantically identical. Since the book seems to be all about the new C++ 17 features it makes sense the author uses it. But be careful: every new featured will be abused untill the dust settles and the common wisdom reaches a common consensus about good and less good ways of using the feature.

In my humble opinion this way just adds confusion (why is there an if construct if the block is executed unconditionally?) and I prefer the simple block. Time will tell if the author's way will become an acceptable or frowned upon construct. Or if nobody will care or if it will forever remain controversial.

Upvotes: 3

Related Questions