Smart pointers: shared and weak.

Hello,

I finished removing NodeLinked. Please check the related issue #18 (closed).

Now I am introducing the shared_ptr and weak_ptr design, following guidelines in:

http://www.umich.edu/~eecs381/handouts/C++11_smart_ptrs.pdf

which are, on the other hand, the regular guidelines -- it's just nicely explained.

I have a skeleton test building the whole WOLF tree from scratch. You can check it out here:

• branch smart_ptr
• file examples/test_sh_ptr.cpp

Easy notes:

• Classes names are just one letter. See the comments in the file. They are P, H, S, p, T, F, C, f, c, M, L.
• By now the test only builds the basic tree, with shared_ptr looking down, and weak_ptr looking up.
• It does not have other pointers, e.g. from Capture C to Sensor S , or from constraints to Frames, Features, or Landmarks.
• It constructs and destructs the whole tree, with each node containing always 2 children:
  • H has 2 sensors, each has 2 processors
  • T has two frames, each has 2 captures, each has 2 features, each has 2 constraints
  • M has 2 landmarks

Less easy notes:

• All members need to be "shared_pointed" to completely avoid dangling objects and non_smart pointers to objects
  • This means that the problem node P needs to be a shared_ptr<P>, not a plain object P.
  • Also, H, T and M are shared pointers in P, not just members; otherwise we cannot point to them from their children.
  • This is what we have been mostly doing in wolf, but now it is an unavoidable must. Please checkout the class P.
• All classes inherit from enable_shared_from_this<T> to be able to establish the links from child to parent. If we find another way to do so, we can remove this inheritance. But this is regular stuff with shared_ptr, we just need to be used to it.
• Because shared_from_this() cannot be called during the constructor of P, P has a method setup() that links H, T and M to P once the constructor of P is finished.
  • Notice that we cannot call setup() inside the constructor of P (this would not make sense),
  • Therefore in the main() we have to construct P, then call setup().

Changed title: Shared pointers → Smart pointers: shared and weak.

Helper notes on shared_ptr:

• Build an object of type T: 2 options (the second is preferred):

shared_ptr<T> t( new T ); /// <-- this works, but uses 2 mallocs, and we might want to avoid the keyword `new`
shared_ptr<T> t ( make_shared<T>() ); /// <-- PREFERRED: this has faster memory allocation, and avoids  `new`

The second method is preferred.

• use a weak_ptr

weak_ptr<T> wp;
shared_ptr<T> sp = wp.lock();
if (sp) // is `sp` a valid pointer?
{
    // use sp
}

• return a pointer to this

class T : public enable_shared_from_this<T>
{
    T(){}
    shared_ptr<T> getThis(){return shared_from_this();}
}

I completed a significant part of the redesign:

• All wolf tree links are now smart pointers

Raw pointers are still used for

• StateBlocks -- EDIT: not anymore, see below
• Things in AudoDiff and code pieces related to Ceres, or the Ceres wrapper
• Something I might have forgotten

I will be merging this work to master soon. @joanvallve , @acoromin and/or @artivis you might want to have a look at the smart_ptr branch.

Tips and tricks about shared_ptr and weak_ptr are on this issue (previous comment).

@jtarraso I would like you to switch to the smart_ptr design, so that we do not diverge any longer. I created a merged branch of your work into smart_ptr, it's called image-shptr.

You can simply:

git checkout image-shptr

and do your work

if something goes wrong, you can simply git checkout your old image branch.

We'll discuss the implications of the smart pointers design next Monday.

Great job
I won't have time today, I will look at it seriously tomorrow.

News:

• All state blocks are now shared_pointed:
  • In nodes S (SensorBase), F (FrameBase) and L (LandmarkBase), as std::vector<shared_ptr<StateBlock> >:
    • the vector allows a random access, which is needed for getPPtr(), getOPtr(), getVPtr() etc. See the methods in S, F, L:
      • getPPtr()
      • getStateBlockPtr()
      • setStateBlockPtr()
      • getStateBlockVec()
  • in derived classes, one adds additional state blocks to the vector, using positions 3 onwards (0 is P, 1 is O, 2 is V). See FrameIMU for an example, where position 3 is for Acc bias, and position 4 for Gyro bias, e.g.,

  inline StateBlockPtr FrameIMU::getAccBiasPtr() const
  {
          return getStateBlockPtr(3);
  }

 - state block registering and unregistering is managed by the node base classes in `S`, `F` and `L`: see the methods: 
     - `registerNewStateBlocks()`
     - `removeStateBlocks()`
- in `P`, as `std::list<shared_ptr<StateBlock> >`, as it was before, but now with shared_ptr

• @joanvallve One exception to the vector<> design in LandmarkBase is the class LandmarkPolyline2D, which uses the vector for the state blocks of base nodes, but a second std::deque for the variable-size list of state blocks for the points in the polyline.
• Although this could all be handled in a single container, the fact that the deque grows in both directions recommends a design as it is, with two separate containers.
• Therefore, methods removeStateBlocks() and registerNewStateBlocks() have been overloaded accordingly.

All is pushed to branch smart_ptr, and will be merged to master this week.

Mentioned in issue #103 (closed)

Merged into master. Two notes:

• The branch smart-ptr is still active since there are still a few raw pointers that require attention. I will continue the work in this branch.
• There is a branch master-raw-ptr hholding the master branch before the merge. Use it if you have trouble with the updated master, although I will delete it soon.

@artivis @joanvallve @acoromin Please try to merge or rebase your work . I will not merge any pull request (even if it is pending) into the old master. @AtDinesh @jtarraso @idelpino I already merged and pushed your branches:

• image
• imu_preintegration, imu_preintegration-check-jacobians
• Hello_Wolf

@joanvallve The remove() methods in the new master branch may not incorporate all the functionality that was implemented in the destruct() methods combined with the ~Destructors().

We need to revise these policies and bring back wolf to the desired operation.

For example, Landmarks that were fixed should not be automatically removed when no constraints point them. This is, I think, not the case in the current master.

Status changed to closed