Move some code to undo.cpp. #5192

pull domob1812 wants to merge 1 commits into bitcoin:master from domob1812:btc-txinundo changing 1 files +36 −24

domob1812 commented at 4:29 PM on November 1, 2014: contributor

Create undo.cpp as new source file and move some code related to undo opreations to it. Also simplify DisconnectBlock a bit.

I guess it is a matter of taste whether one likes these changes or not. But I think it makes sense to move the code now in CTxInUndo::Apply there and out of DisconnectBlock. It makes both functions easier to read, IMHO.

in src/undo.cpp:None in af6f33c549 outdated

   9 | +#include "coins.h"
  10 | +#include "main.h"
  11 | +#include "streams.h"
  12 | +#include "util.h"
  13 | +
  14 | +#if defined(NDEBUG)

Diapolo commented at 4:45 PM on November 1, 2014:

Not sure this part belongs in undo.

sipa commented at 4:50 PM on November 1, 2014: member

Moving code out makes perfect sense, but please don't create circular dependencies (between main and undo, in this case).

See #5111 for an alternative that moves the methods with implementation to main, and all header definitions to undo.h (including CBlockUndo).
sipa commented at 4:53 PM on November 1, 2014: member

I see you're also moving code out of DisconnectBlock, which makes sense. But can you leave that as a function in main, rather than a method in undo? We're aiming in general to have a separation between files that purely define data structures and serializations (core/transaction.h, core/block.h, undo.h, script/script.h, serialize.h, ...), and others that operate on their validity and database interactions, so that subsets of the code can be built without including validation-related code.
domob1812 commented at 5:08 PM on November 1, 2014: contributor

I understand that. So what about only creating CTxInUndo::Apply with implementation in main.cpp and without creating undo.cpp at all?
sipa commented at 5:11 PM on November 1, 2014: member

No, please keep implementations in the corresponding .cpp file of the .h file where things are defined. Not doing so makes it impossible to reason about dependencies (you could end up having code that needs main.o or not, just depending on whether you use a particular method defined in an unrelated file).
domob1812 commented at 5:33 PM on November 1, 2014: contributor

Ok. So if you want to keep the code in main, it will be a standalone method ApplyTxInUndo and not part of class CTxInUndo. That's also perfectly possible. Is this what you have in mind?
sipa commented at 7:16 PM on November 1, 2014: member

To me that sounds fine yes - it also avoids having undo.h depend on coins.
domob1812 force-pushed on Nov 2, 2014
domob1812 commented at 10:58 AM on November 2, 2014: contributor

Updated accordingly.
sipa commented at 4:03 PM on November 3, 2014: member

utACK, verified move-only.
domob1812 force-pushed on Nov 13, 2014

Split logic to undo txin's off DisconnectBlock.

Instead, create a separate function that applies the undo operation of a
CTxInUndo object onto a CCoinsViewCache.  This method is used from
DisconnectBlock.

7b5331a606

domob1812 force-pushed on Nov 18, 2014
fanquake commented at 8:35 AM on January 23, 2015: member

Needs rebase.
domob1812 force-pushed on Jan 23, 2015
domob1812 commented at 1:56 PM on January 23, 2015: contributor

Rebased. The new version is here: #5699 See there for an explanation why I had to create a new pull request. (They are identical.) Closing this.
domob1812 closed this on Jan 23, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#5699 Split logic to undo txin's off DisconnectBlock.