Kratos icon indicating copy to clipboard operation
Kratos copied to clipboard

[Core] Fixing const-correctness in kratos pointers

Open EduardGomezEscandell opened this issue 3 years ago • 8 comments

📝 Description There are many const pointer that should be pointer to const. See https://github.com/KratosMultiphysics/Kratos/issues/9779 for a more detailed explanation.

🆕 Changelog Defined:

  • PointerToConst (shared)
  • PointerToConst (intrusive)
  • SharedPointerToConst
  • WeakPointerToConst
  • UniquePointerToConst

Fixed return type of "const pGetters" from const Type::Pointer to Type::PointerToConst.

Breaking changes The breaking changes were fixed in the applications. Users of Kratos outside the repository may need to remove const from some elements, conditions and modelparts to acomodate these changes. To avoid any chaos, a deprecation warning will be added for some time before merging this change. See #9790.

Unfixed issue Method Clone of elements and conditions should not be const, since it includes a call to the non-const version of pGetProperties.

Fixing this would be too much (it would break practically every single element and condition). To avoid doing this work, I decided too keep pGetProperties with a the wrong return type Properties::Pointer rather than Properties::PointerToConst.

This is left outside the scope of this PR.

EduardGomezEscandell avatar Mar 22 '22 10:03 EduardGomezEscandell

@KratosMultiphysics/technical-committee has been discussing about this again.

We are in favour of the change as current behaviour might be misleading.

However, we're afraid of how this propagates through the code. In this regard, we would like to know the opinion of the @KratosMultiphysics/implementation-committee.

rubenzorrilla avatar Feb 24 '23 11:02 rubenzorrilla

Speaking on my own (to be discussed in the @KratosMultiphysics/implementation-committee ), I agree that the current notation is confusing (I was not sure of this behaviour until now!) so I would vote in favour of changing it. Especially after reading the very insightful https://github.com/KratosMultiphysics/Kratos/issues/9779 Thank you @EduardGomezEscandell

AlejandroCornejo avatar Feb 24 '23 12:02 AlejandroCornejo

Ping @KratosMultiphysics/implementation-committee

pooyan-dadvand avatar May 30 '23 08:05 pooyan-dadvand

I'm also on board for const-correctness, but making that happen properly will be a pretty big undertaking and will definitely lead to extensive changes and API breaks. If we're doing it, we ought to do it right all at once to avoid breaking everyone's branches/forks all the time.

matekelemen avatar May 30 '23 08:05 matekelemen

@KratosMultiphysics/technical-committee agrees about this change, however we think that @KratosMultiphysics/implementation-committee should take the lead on finishing it.

pooyan-dadvand avatar Jun 16 '23 08:06 pooyan-dadvand

Feel free to start it from zero, this branch is probably a bit too old now.

If you chose to continue on this branch, or use it as inspiration, don't hesitate to ask about any of the changes I made (although I can't promise I'll remember the reasoning 😅).

EduardGomezEscandell avatar Jun 16 '23 08:06 EduardGomezEscandell

Hey, we still want to merge this. reopening.

*Note: We want to keep the PR open so the branch does not get deleted by accident.

roigcarlo avatar May 15 '24 11:05 roigcarlo

Oh I thought you were starting your implementation from scratch :sweat_smile:.

I was just trying to remove my old PRs.

EduardGomezEscandell avatar May 15 '24 12:05 EduardGomezEscandell