Change pointer to shared_ptr #671

bam241 · 2020-04-09T15:01:49Z

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 · 2020-04-09T18:43:45Z

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 · 2020-04-09T18:50:18Z

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.

src/dagmc/DagMC.cpp

pshriwise · 2020-04-09T19:07:42Z

src/dagmc/DagMC.cpp


  // make new GeomTopoTool and GeomQueryTool
-  GTT = new moab::GeomTopoTool(MBI, false);
-  GQT = new moab::GeomQueryTool(GTT, overlap_tolerance, p_numerical_precision);
+  GTT = std::shared_ptr<GeomTopoTool> (new GeomTopoTool(MBI.get(), false));


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)?

src/dagmc/tests/dagmc_simple_test.cpp

pshriwise · 2020-04-09T19:10:31Z

src/dagmc/tests/dagmc_simple_test.cpp

  delete dagmc;
-  delete mbi;


src/dagmc/tests/dagmc_simple_test.cpp

src/dagmc/DagMC.cpp

bam241 · 2020-04-10T04:27:50Z

@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.

src/dagmc/DagMC.cpp

pshriwise · 2020-04-10T13:10:28Z

src/dagmc/DagMC.hpp

-  GeomTopoTool* geom_tool() {return GTT;}
+  [[ deprecated("Replaced by std::shared_ptr<GeomTopoTool> geom_tool_ptr()") ]]
+  GeomTopoTool* geom_tool() {return GTT.get();}
+  std::shared_ptr<GeomTopoTool> geom_tool_ptr() {return GTT;}


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.

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 · 2020-04-10T13:12:29Z

src/dagmc/DagMC.hpp

  Interface* moab_instance() {return MBI;}
+  std::shared_ptr<Interface> moab_instance_ptr() {return MBI_shared_ptr;}


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

Co-Authored-By: Patrick Shriwise <[email protected]>

pshriwise · 2020-04-11T19:48:31Z

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

bam241 · 2020-04-11T20:35:26Z

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 · 2020-04-27T17:43:03Z

@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 · 2020-04-27T17:45:05Z

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 :)

…ared_ptr and only instanciated as a raw pointer

bam241

@svalinn/dagmc I am now satisfied with this.

I updated the news to reflect our discussion.

bam241 · 2020-04-27T18:28:32Z

src/dagmc/DagMC.hpp

+  std::shared_ptr<Interface> moab_instance_sptr() {
+    if (nullptr == MBI_shared_ptr)
+      std::runtime_error("MBI instance is not defined as a shared pointer !");
+    return MBI_shared_ptr;
+  }


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 · 2020-04-30T13:41:45Z

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 · 2020-04-30T13:52:38Z

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 · 2020-04-30T19:40:40Z

Thanks again for tackling this @bam241!

bam241 added 18 commits April 7, 2020 12:00

change point to uniq/shared ptr

2808dc5

turned MBI into a unique_ptr

a2d6f24

firace add c++ 11

c5a954b

no need to force it

b93dc9b

switching from unique to shared

ab729dc

forcing c++11

d483256

Add indent

9d955aa

removing it ?

f31eb12

revseting the DAMC_macro

2f17c8c

empty commit

967353b

foircing back c++11

67ffa6c

add c++11 in test

b78c085

convert from pointer to shared_ptr...

e415a8f

revert to std c++11 cmake setup

c147bd0

revert to force c++11

70de322

readd previous option

02c2a98

and now ?

8332b18

this is clean

b4b3029

bam241 added Type: Enhancement Type: Style & Best Practice 5-InReview PLEASE_REVIEW labels Apr 9, 2020

bam241 requested review from gonuke, makeclean and pshriwise April 9, 2020 15:01

bam241 changed the title ~~Change point declaration to shared_ptr~~ Change pointer to shared_ptr Apr 9, 2020

adding news

2c48814

pshriwise requested changes Apr 9, 2020

View reviewed changes

bam241 added 5 commits April 9, 2020 20:33

adding new getter for share_ptr, and deprecate old ones

5339237

changed test to shape_ptr

73b5eaf

change raw pointer for all shared_ptr in dagmc test

e5204af

fixing style

8e9a2bd

remove unnecessary includes

075cec7

update news

5715833

pshriwise requested changes Apr 10, 2020

View reviewed changes

bam241 and others added 2 commits April 10, 2020 09:44

Update src/dagmc/DagMC.cpp

2dd8c83

Co-Authored-By: Patrick Shriwise <[email protected]>

do not return shared_ptr anymore

ff99edc

bam241 added 6 commits April 13, 2020 13:30

change to unique ptr

407c83d

attempt to fix unique_ptr

cb79661

thx @pshriwise for solving that one !

53f5110

now return a shared_ptre for GTT and allows to retrieve MBI shared_ptr

59dbae4

set MBI_shared_ptr ass a nullptr when MBI passed as raw pointer

96e9aca

return the correct MBI version

24317b0

pshriwise approved these changes Apr 27, 2020

View reviewed changes

bam241 added 2 commits April 27, 2020 13:14

reorder incldues, add runtime_error when trying to return MBI as a sh…

c7e7a8c

…ared_ptr and only instanciated as a raw pointer

update news details

3088944

bam241 commented Apr 27, 2020

View reviewed changes

pshriwise approved these changes Apr 27, 2020

View reviewed changes

pshriwise merged commit 69e699b into svalinn:develop Apr 30, 2020

bam241 deleted the shared_ptr branch November 28, 2024 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change pointer to shared_ptr #671

Change pointer to shared_ptr #671

bam241 commented Apr 9, 2020 •

edited

Loading

pshriwise commented Apr 9, 2020 •

edited

Loading

pshriwise commented Apr 9, 2020

pshriwise left a comment

pshriwise Apr 9, 2020

pshriwise Apr 9, 2020

bam241 commented Apr 10, 2020

pshriwise left a comment

pshriwise Apr 10, 2020 •

edited

Loading

bam241 Apr 10, 2020 •

edited

Loading

pshriwise Apr 10, 2020

pshriwise commented Apr 11, 2020 •

edited

Loading

bam241 commented Apr 11, 2020

pshriwise left a comment

bam241 commented Apr 27, 2020

pshriwise commented Apr 27, 2020

bam241 left a comment •

edited

Loading

bam241 Apr 27, 2020 •

edited

Loading

bam241 commented Apr 30, 2020 •

edited

Loading

pshriwise commented Apr 30, 2020

pshriwise commented Apr 30, 2020

		Interface* moab_instance() {return MBI;}
		std::shared_ptr<Interface> moab_instance_ptr() {return MBI_shared_ptr;}

Change pointer to shared_ptr #671

Change pointer to shared_ptr #671

Conversation

bam241 commented Apr 9, 2020 • edited Loading

pshriwise commented Apr 9, 2020 • edited Loading

pshriwise commented Apr 9, 2020

pshriwise left a comment

Choose a reason for hiding this comment

pshriwise Apr 9, 2020

Choose a reason for hiding this comment

pshriwise Apr 9, 2020

Choose a reason for hiding this comment

bam241 commented Apr 10, 2020

pshriwise left a comment

Choose a reason for hiding this comment

pshriwise Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

bam241 Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

pshriwise Apr 10, 2020

Choose a reason for hiding this comment

pshriwise commented Apr 11, 2020 • edited Loading

bam241 commented Apr 11, 2020

pshriwise left a comment

Choose a reason for hiding this comment

bam241 commented Apr 27, 2020

pshriwise commented Apr 27, 2020

bam241 left a comment • edited Loading

Choose a reason for hiding this comment

bam241 Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

bam241 commented Apr 30, 2020 • edited Loading

pshriwise commented Apr 30, 2020

pshriwise commented Apr 30, 2020

bam241 commented Apr 9, 2020 •

edited

Loading

pshriwise commented Apr 9, 2020 •

edited

Loading

pshriwise Apr 10, 2020 •

edited

Loading

bam241 Apr 10, 2020 •

edited

Loading

pshriwise commented Apr 11, 2020 •

edited

Loading

bam241 left a comment •

edited

Loading

bam241 Apr 27, 2020 •

edited

Loading

bam241 commented Apr 30, 2020 •

edited

Loading