Skip to content

Replace manually-deleted owning raw pointers with std::unique_ptr - #763

Open
lyskov-ai wants to merge 1 commit into
RosettaCommons:mainfrom
lyskov-ai:refactor/owning-raw-pointers-to-unique-ptr
Open

lyskov-ai wants to merge 1 commit into
RosettaCommons:mainfrom
lyskov-ai:refactor/owning-raw-pointers-to-unique-ptr

Conversation

@lyskov-ai

Copy link
Copy Markdown
Contributor

Twelve classes held a heap object by raw pointer and freed it with an explicit delete in their destructor. This holds each in a std::unique_ptr instead, so ownership is expressed in the type and the destructors stop hand-managing memory.

Graph edge pools (8 classes)

These all own one or two boost::unordered_object_pool instances with exactly the same idiom — allocate in the constructor initializer list, delete and null out in the destructor:

Class Pool member(s)
utility::graph::Graph edge_list_element_pool_, edge_pool_
utility::graph::Digraph edge_list_element_pool_, edge_pool_
core::scoring::EnergyGraph energy_edge_pool_
core::scoring::MinimizationGraph minimization_edge_pool_
core::scoring::TenANeighborGraph tenA_edge_pool_
BuriedUnsatPenaltyGraph bunsat_edge_pool_
protocols::jd3::JobDigraph job_edge_pool_
protocols::legacy_sewing::SewGraph hash_edge_pool_

In Graph and Digraph this also corrects the teardown order. The explicit delete freed the element pool from the destructor body, while edge_list_ — which holds a reference to that pool and returns its elements to it as it is destroyed — was still alive. It only worked because delete_everything() empties the list first. Member destruction now runs the list first and frees the pool afterwards, so the ordering is guaranteed by declaration order rather than by that coincidence; the declaration order dependency is noted in a comment on the member.

The pools are constructed the same way as before (new in the initializer list), so the constructors are unchanged, and the pools are not serialized (already marked EXEMPT in the cereal save/load), so serialization is unaffected.

Single owned objects (4 classes)

SICFast (h1_, h2_), MotifHashRigidScore (reshash_), protocols::viewer::triangleIterator (gradPtr) and FragmentCandidate (pool_name_) each owned a single object while leaving their copy constructor and copy assignment implicit — copying any of them would have duplicated the pointer and double-freed it. The unique_ptr members make them non-copyable, so that hazard is now a compile error rather than a latent crash. Nothing in the tree copies them; the debug build is clean.

triangleIterator::densityPtr is a non-owning observer pointer and is deliberately left as a raw pointer.

MotifHashRigidScore::ssinfo1_ and ssinfo2_ were never allocated — initialized to null, never assigned, and their only readers are commented-out lines — so they are deleted rather than converted, along with the now-unused SS_Info2 includes.

Deliberately not included

Three other families of manual delete in destructors were left out because their fix shape and risk profile genuinely differ from a unique_ptr swap:

  • delete[] array owners (packstat::Array2D, ReplicaExchangeMC, BiasEnergy::Histogram, the MPI pool buffers) need std::vector, not unique_ptr, and several sit behind MPI build guards.
  • EdgeList/DirectedEdgeList::end_, the sentinel node of the intrusive circular edge list. Copy is already deleted on those classes so there is no bug, and converting would add .get() at roughly 58 use sites in a hot data structure for no correctness gain.
  • Vectors of owning node pointers (InteractionGraphBase::ig_nodes_, AdditionalBackgroundNodesInteractionGraph::bg_nodes_, DoubleLazyEdge::two_body_energies_), which hand out raw NodeBase*/FArray2D* through their public APIs and would need a much wider change.

This keeps the diff at ~110 lines rather than the usual larger bundle; the remaining candidates are better as separate PRs than mixed in here.

Twelve classes held a heap object by raw pointer and freed it with an
explicit delete in their destructor. Hold each in a std::unique_ptr
instead, so ownership is expressed in the type and the destructors stop
hand-managing memory.

Eight graph classes own boost::unordered_object_pool instances:
utility::graph::Graph and Digraph (element pool plus edge pool each),
EnergyGraph, MinimizationGraph, TenANeighborGraph,
BuriedUnsatPenaltyGraph, JobDigraph and SewGraph. In Graph and Digraph
this also corrects the teardown order: the explicit delete freed the
element pool from the destructor body while edge_list_ -- which holds a
reference to that pool and returns its elements to it as it is destroyed
-- was still alive. Member destruction now runs the list first and frees
the pool afterwards.

SICFast, MotifHashRigidScore, triangleIterator and FragmentCandidate
each owned a single object while leaving their copy operations implicit,
so copying any of them would have double-freed. The unique_ptr members
make them non-copyable.

MotifHashRigidScore::ssinfo1_ and ssinfo2_ were never allocated -- always
null, with their only readers commented out -- so they are deleted rather
than converted, along with the now-unused SS_Info2 includes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant