Change pointer to shared_ptr

[bam241]

addressing comment from #660:

Rather than manually `new`'ing and `deleting` these objects, I would recommend 
using a RAII ("Resource Allocation Is Initialization") technique to manage their 
memory [1].  Specifically, you can use smart pointers [2] such as `std::unique_ptr` 
or `std::shared_ptr` to allocate `GTT` and `GQT` in the constructor, and subsequently 
not be responsible for their deletion.

Unless you are allocating a computationally large number of pointers (such as in a 
core data structure in a game engine), it's typically recommended to use a smart 
pointer when possible to guard against memory leaks.

[1] https://en.cppreference.com/w/cpp/language/raii
[2] https://en.cppreference.com/book/intro/smart_pointers

Originally posted by @PullRequest-Agent in #660

Targeting declaration and deletion of pointers in DagMC() (see DAGMC.cpp L64)

Not sure we want to change the API (constructor of DagMC class) but I was not able to have shared_ptr<> in the body of the DagMC class and still accept a normal pointer for the Interface* because when declaring the point outside of the DagMC instance, one need to delete it, but the ownership of the Interface* has been transferred to the shared_ptr<> which is suppose to take care of the deallocation.
But it is not obvious without looking at the DagMC.cpp source code that you should not deallocate your own pointer.
More over if the Interface* object would probably been deallocated after deleting the DagMC object which will be problematic if you want to used it ( Interface*) after the DagMC deletion....

---

I created a separate PR for this one, as it will be simpler to review, and I think it might need some discussion about how to address this.

To make that change I was not able to find an other implementation, so I see only 2 solutions:

• keep the code as it is with pointer and delete
• slightly change the API

[pshriwise]

These are great changes IMO! As you noted, @bam241, requiring use of a std::shared_ptr for the MOAB interface makes DAGMC a little more rigid.

One thought might be to have a raw pointer to a moab::Interface in the DAGMC class which is set regardless of the constructor used and be returned by the DAGMC::moab_instance() method. Our constructors taking a raw moab::Interface pointer would then look like this:

DagMC::DagMC(Interface* mb_impl, double overlap_tolerance, double p_numerical_precision) {
  // if we arent handed a moab instance, create one
  if (nullptr == mb_impl) {
   mb_shared = std::shared_ptr<Core>();
   MBI = mb_shared.get();
  } else {
   MBI = mb_impl;
  }
  
  // make new GeomTopoTool and GeomQueryTool
  ...
}

If the pointer is passed in, we won't delete it on cleanup but if we did create it, the std::shared_ptr will take care of itself when the DagMC object is destroyed. No new's /delete's to worry about on our end.

Does that sound plausible or am I missing something?

Clarification:

In the example above MBI is still our raw moab::Interface* and we would add an attribute called mb_shared or something similar for the shared_ptr.

[pshriwise]

Adding to the comment above, if we go this route I think it would be good for us to include a constructor that takes in a std::shared_ptr<Interface> and start a discussion about marking the other constructor as deprecated or indicate some kind of preference in docs/comments for the constructor taking a shared_ptr. IMO we want to promote the use of these modern C++ practices when possible.

[pshriwise]

Thanks for bringing us up-to-date on this front @bam241! A few suggestions on creating the std::shared_ptr's.

[pshriwise]

Should these be unique_ptr's as those objects are going to carry state that is related directly to calls that have been made on this DagMC object (e.g. init_obbs)?

[pshriwise]

🎉

[bam241]

@gonuke @pshriwise @makeclean this should be ready for an other round of review

[pshriwise]

A couple of thoughts on returning shared pointers from the DagMC interface. I think we're going to run into trouble making sure they don't cause problems at this point. We'll get there someday though hopefully.

[pshriwise]

We need to be careful about passing back shared pointers for these objects. In the case where we've created the MOAB instance, these objects aren't guaranteed to be useable beyond the life of the DAGMC object.

// populates the MBI_shared_ptr and sets MBI
std::unique_ptr<DagMC> dag = std::make_unique<DagMC>();
moab::Errorcode rval = dag->load_file("test.h5m");
MB_CHK_ERR(rval);

rval = dag->init_OBBTree();
MB_CHK_ERR(rval);

// retrieve a shared_ptr GTT from the DagMC instance
std::shared_ptr<GeomTopoTool> gtt = dag->geom_tool_ptr();

// MBI_shared_ptr is cleaned up and the MOAB instance is destroyed as intended
dag.reset();

// the GTT shared_ptr is still available, but any call that relies on it's underlying MBI pointer will fail
rval = gtt->check_model(); // will segfault
MB_CHK_ERR(rval);

I think it would be better to keep only the geom_tool() method for now.

[bam241]

it make sense the only shared_ptr we shall return is the MBI one....
the other one depends on each other through the raw pointer not the shared pointer, so we can't guaranty they will be working outside of the DagMC instance....

[pshriwise]

This method is also a little inconsistent with our current design. What if the deprecated constructor is used and this object is a nullptr?

[pshriwise]

This is really close I think. Any luck with making the GQT and GTT unique pointers?

[bam241]

This is really close I think. Any luck with making the GQT and GTT unique pointers?

Sadly not, GQT is an argument of GTT so it won't be possible.

I have a error message with the GTT that I don't quite understand, I'll try again an post the msg...

(when I googled it, the easy work around was to use a shared_ptr :P )

[pshriwise]

Thanks again @bam241 for taking this on! It's a much-needed update. Hopefully we can start to convert some of our other classes to these more modern practices in the future.

[bam241]

@pshriwise @gonuke @makeclean
Looking back at this, we have a return method for the MBI_shared_ptr.
it will be a nullptr if the MBI instance has been passed by the user as a raw_ptr and populated otherwise.

Do we want to throw a warning if someone if trying to get it and it is not defined, or is returning a nullptr enough ?

[pshriwise]

I'd prefer there be some kind of warning/error here. If we're returning an empty shared pointer and know it, it seems as though we should give some kind of indication about that. Honestly, I think we should throw a runtime error, but I'd take a warning too :)

[bam241]

@svalinn/dagmc I am now satisfied with this.

I updated the news to reflect our discussion.

[bam241]

Added a runtime_error when trying to return the MBI instance as a shared_ptr when it has been provided as a direct pointer. (It has been implemented in the Header, as it is fairly simple, I can move it to the cpp file if you prefer)

[bam241]

A remaining identified issue (that existed before) is if returning GTT instance while having the MBI instance instantiated internally, if one delete the DagMC object, the MBI pointer will be deallocated... leaving the GTT pointing to a void pointer...

This would also occur before this change tho... (Does not mean we should not fix it, but might be outside of the scope of this PR...)

[pshriwise]

Agreed @bam241. Let's make an issue for that and move forward for now. I'll merge this today if there are no other comments/concerns.

[pshriwise]

Thanks again for tackling this @bam241!