Misc. cleanup found while mucking with search engines code:

sql/transaction.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sql/transaction.h"
 
 #include "base/logging.h"
 #include "sql/connection.h"
 
 namespace sql {
 
 Transaction::Transaction(Connection* connection)
     : connection_(connection),
       is_open_(false) {
 }
 
 Transaction::~Transaction() {
   if (is_open_)
     connection_->RollbackTransaction();
 }
 
 bool Transaction::Begin() {
-  if (is_open_) {
-    NOTREACHED() << "Beginning a transaction twice!";
-    return false;
-  }
+  DCHECK(!is_open_) << "Beginning a transaction twice!";

[Scott Hess - ex-Googler, 2014/03/31 18:09:08]

These changes may cause actual incorrect changes in production which might be
wrong.  For instance, previously Begin() wouldn't stack another transaction in
the connection, now it will, and previously it could fail where now it would
succeed so the caller may take further actions based on that.  Rollback() and
Commit() previously wouldn't forward an incorrect request to the connection, now
they will.  Due to how these things interact implicitly in nested fashion, the
affected parties may not actually realize there's another transaction around
that could break them.  Since the destructor only calls rollback if a
transaction is open, a double-Begin() would not result in everything being
unwound unless the caller is making all the correct explicit unwinding calls
(though it probably goes without saying that the caller is doing something wrong
at this point).  I'm not sure it's a good idea to make this change.

On the other hand, I cannot think of anyplace where I've seen Transaction being
used in a way which would hit any of these issues.  My argument is more that the
Transaction is not constructed in a way which avoids all of these issues.

[Peter Kasting, 2014/03/31 19:53:56]

We have to make _some_ change, because the old code is wrong -- it handles
DCHECK failure, which is banned.  The old code claims that our code never
misuses Transactions in these ways, meaning that it's guaranteed safe to make
the change I'm making.  If that guarantee is violated, we already have bugs in
our code.  I'd rather not bandaid them.

Because failing these checks is purely a matter of actual coding errors
(misusing transaction semantics), it seems reasonable to demand that people use
transactions properly.  (By contrast, we couldn't do this with e.g. the return
values of attempts to execute SQL statements, since error cases like disk
corruption could always mean we get a real-world failure.)
All classes have two choices: they can be defensive and ensure they handle all
possible misuse, or they can pass that responsibility to their callers, and
assert that they are used properly.  Normally, the second seems preferable,
since it is generally both simpler and more conducive to all callers actually
being correct (we won't have people writing grossly wrong code and having the
callee "handle it", then having that code get copied elsewhere).

All this is a long way of saying that, while I'm willing to instead remove the
NOTREACHED()s and leave the return statements, I think the change I'm making
here is significantly better.

And I've never seen code that would hit this, either.

   is_open_ = connection_->BeginTransaction();
   return is_open_;
 }
 
 void Transaction::Rollback() {
-  if (!is_open_) {
-    NOTREACHED() << "Attempting to roll back a nonexistent transaction. "
-                 << "Did you remember to call Begin() and check its return?";
-    return;
-  }
+  DCHECK(is_open_) << "Attempting to roll back a nonexistent transaction. "
+                   << "Did you remember to call Begin() and check its return?";
   is_open_ = false;
   connection_->RollbackTransaction();
 }
 
 bool Transaction::Commit() {
-  if (!is_open_) {
-    NOTREACHED() << "Attempting to commit a nonexistent transaction. "
-                 << "Did you remember to call Begin() and check its return?";
-    return false;
-  }
+  DCHECK(is_open_) << "Attempting to commit a nonexistent transaction. "
+                   << "Did you remember to call Begin() and check its return?";
   is_open_ = false;
   return connection_->CommitTransaction();
 }
 
 }  // namespace sql