Page MenuHomePhabricator

Clear container after move
ClosedPublic

Authored by castarco on Jan 19 2019, 12:27.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGINGff5296fa2cfb: Clear container after move
rABCff5296fa2cfb: Clear container after move
Summary

Clear parents after move in
DisconnectedBlockTransactions::addForBlock to ensure a known state
before reusing the variable (the standard enforces a valid state, but
not necessarely a known & specific one).

Signed-off-by: Andres Correa Casablanca <andres@thirdhash.com>

Test Plan

Running the current tests should be enough.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a reviewer: Restricted Owners Package.Jan 19 2019, 12:27
src/txmempool.h
398 ↗(On Diff #6743)

Removing the const qualifier allows the instances of this class to be movable.

deadalnix requested changes to this revision.Jan 19 2019, 13:11
deadalnix added inline comments.
src/txmempool.cpp
1333 ↗(On Diff #6743)

Fair enough, even though std::move is known to leave the moved datastructure empty, I don't think the spec enforces it.

src/txmempool.h
398 ↗(On Diff #6743)

Changing these midway would be catastrophic. This is a good constraint to enforce.

Implementing a move constructor seems like a better approach.

This revision now requires changes to proceed.Jan 19 2019, 13:11
src/txmempool.h
398 ↗(On Diff #6743)

Unfortunately, the move constructor does not fully solve the problem, there's need for the move assignment operator, which is automatically deleted by the existence of the const qualifier.
I think removing this qualifier is not very dangerous because this properties are private and the class does not have methods with side effects declared.

Another option is to undo this change and enforce a known state on parents by calling parents.clear().

src/txmempool.h
398 ↗(On Diff #6743)

That sounds like a better solution as we keep enforcing invariant, and the compiler is likely to see through it anyways.

Call clear instead of swapping variables

castarco retitled this revision from Avoid using variable after move to Clear container after move.Jan 21 2019, 10:06
castarco edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Jan 21 2019, 16:57
This revision was automatically updated to reflect the committed changes.