Skip to content
Snippets Groups Projects

WIP: Make getiterationstep return shared ptr

The method IterativeSolver::getIterationStep should return a shared_ptr rather than a reference. Since IterationStep is an abstract base class, code that calls getIterationStep frequently does dynamic casting afterwards. You cannot do that with a reference though, unless using some trickery.

Unfortunately, simply changing the return type of the method is not backward compatible. This is why this merge request is marked as WIP. Any ideas on how to proceed?

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Developer

    Can't one just dynamic_cast the address of the returned reference? At least this is how it is done in the classes in dune-solvers that call the getIterationstep method.

  • I know, but I want to assign the casted result to a new shared_ptr. I am scared that dynamic_casting the address of the reference will lead to ownership screwup.

  • It will! That's the reason why std::dynamic_pointer_cast exists: Cast the pointer but share the reference count.

  • Developer

    Concerning backward compatibility, we could simply make the dune-solvers 2.5.2 release (which does not exist jet) with the old getter and change the method in the master afterwards? Or would you want the change to happen there as well?

  • Developer

    Or 2.6 release

  • @graeser I don't understand you. When I take the address of a reference (as is currently done), then what I get is a C pointer, which I cannot use with dynamic_pointer_cast. I want shared_ptr as the return value of getIterationStep exactly because I want to apply dynamic_pointer_cast to it.

  • Developer

    I understood @graeser's comment as an answer to the uncertainty whether dynamic casting the address can screw up the ownership, not as a solution to the current problem.

    Btw. is stackobject_to_shared_ptr helping to avoid the ownership problem in your case? (it should at least in the case where the IterativeSolver lives longer than your own shared_ptr)

  • Let's not discuss several things at once. Do you guys agree in principle that the method should return shared_ptr? Then all we need to take about is how to get there.

  • Developer

    I actually would prefer keeping it a reference for consistency with the other getters in dune-solvers

    In case you absolute need a pointer I would rather add a second method like getIterationStepPtr

  • @oliver.sander_at_tu-dresden.de You said that you're scared that plain dynamic_cast may screwup ownership. I only wanted to highlight, that it will screwup ownership if you store this in a shared_ptr again.

    Regarding the original issue: The question if one should hand out a shared_ptr is IMO solely an ownership question. If you want to allow sharing owner ship to user code, then hand out the shared_ptr. If the answer you don't want this, but pointer syntax, then return a raw pointer.

    If it's only about the RTTi syntax: I don't see why you cannot dynamic_cast a reference. You could even drop the assertions, because a failed dynamic_cast on a reference will throw an exception (regardless of debug flags).

  • BTW: Exporting shared_ptrs easily screws up const-correctness, because there's various interpretations on what this could mean. Either const means non-mutable ownership or non-mutable value. Classic const-correctness would be the former, semantically I'd opt for the latter. Since you export a shared_ptr copy and no reference, I suspect you intend the latter which means you need shared_ptr<const T> instead of const shared_ptr<T>.

    1. I do want to share ownership with user code.

    2. Yes, my code should be changed to shared_ptr<const T>

  • added 64 commits

    • c7fa9269...8ecddd67 - 62 commits from branch master
    • 210ce85f - Make getIterationStep return std::shared_ptr instead of &
    • 36fa3779 - getIterationStep returns a shared_ptr now

    Compare with previous version

Please register or sign in to reply
Loading