Deprecate ToUniquePtr.

It was just introduced to allow to show a deprecation warning in case someone consumes a GridFactory::createGrid() into a raw-pointer.

@simon.praetorius Instead of !911 (closed), is this what you meant?

mentioned in merge request !911 (closed)

@simon.praetorius Ping. Can you please confirm that this MR is what you prefer over !911 (closed)?

Yes, This is what I had in mind. I will test it.

dune-grid has several occurrences. We should clean up these first.

Yes, probably some grid readers use this toUniquePtr. Also the GridFactory?

The most difficult part is the interface function in the GridFactory, i.e., https://gitlab.dune-project.org/core/dune-grid/-/blob/master/dune/grid/common/gridfactory.hh#L201 If we change the interface, then all implementations must also be changed. There is no transition time. This was my argument for dune-grid#98 (closed). We cannot provide two functions with one deprecated in the interface. Either it is changed or not.

So, when deprecating ToUniquePtr we should also change the lines in GridFactory and all the implementations of the GridFactory for the different grids. Also in the external grid managers this change in GridFactory will break the code. But this is what we have to live with. I do not see an alternative.

If external code breaks anyway, deprecating doesn't make sense, right?

We should prepare an according merge request and discuss when merging them in a wider group.

One could, maybe, replace the class by an alias template:

template <class T>
using ToUniquePtr [[deprecated]] = std::unique_ptr<T>;

template <class T, class... Args>
[[deprecated]]
std::unique_ptr<T> makeToUnique(Args&&... args)
{
  return std::make_unique(std::forward<Args>(args)...);
}

And then advertise to use std::unique_ptr instead. This could also be updated in all the other core modules.

The original intend was to deprecate usage with raw pointers, e.g. the following was deprecated:

Grid* grid = factory.createGrid()

This will (and should) give an error with the proposed changes above.

The to_unique_ptr_test.cc should simply be removed, since we do not implement anything when using the aliases.

mentioned in issue #251 (closed)

added 1 commit

• e66aba4c - Simplify ToUniquePtr deprecation

Compare with previous version

Do we have to adjust dune-grid before merging this? Should we include the broader Dune community to give them a heads-up what we are planning?

In grid implementation one gets a deprecation warning for the header and the template now, e.g., in FoamGrid

#if DUNE_VERSION_LT(DUNE_COMMON, 2, 7)
  FoamGrid<dimgrid, dimworld, ctype>* createGrid() override {
#else
  ToUniquePtr<FoamGrid<dimgrid, dimworld, ctype>> createGrid() override {
#endif

This needs a third case:

#if DUNE_VERSION_LT(DUNE_COMMON, 2, 7)
// no include needed
#elif DUNE_VERSION_LT(DUNE_COMMON, 2, 8)
#include <dune/common/to_unique_ptr.hh>
#else 
#include <memory>
#endif

// ...

#if DUNE_VERSION_LT(DUNE_COMMON, 2, 7)
  FoamGrid<dimgrid, dimworld, ctype>* createGrid() override {
#elif DUNE_VERSION_LT(DUNE_COMMON, 2, 8)
  ToUniquePtr<FoamGrid<dimgrid, dimworld, ctype>> createGrid() override {
#else 
  std::unique_ptr<FoamGrid<dimgrid, dimworld,ctype>> createGrid() override {
#endif

But, this problem was introduced already with the ToUniquePtr class from the beginning.

Maybe this should be communicated to the developer mailinglist, even with an example. Not, the current code should still work, but just raises a warning. At least the library code should work. If a user has ignored the deprecation warning of the ToUniquePtr class, she will get a hard error now.

I am not sure I understand your code right, but are you aiming to get this change done after Dune 2.8? I would do it now - after a discussion on the mailing list, meaning before 2.8.
To make the code less complicated, I would unconditionally include <memory> and just add to_unique_ptr.hh if people want to stay compatible to 2.7.

What do you think?

I repeat my question, is this before Dune 2.8 or after? What do you think about my proposed reduction of your code?

I think, it should be merged now, before the 2.8 release. It puts the deprecation of that transition function to a new level. So, that after 2.9 is can finally be removed completely.

+1 for merging pre-2.8.

Yes please, pre-2.8. Simon, do you mind announcing this on the mailing list and explaining what Dune grids have to do to adjust to this change?

mentioned in merge request dune-grid!497 (merged)

In dune-grid!497 (merged) the type is removed from dune-grid

assigned to @simon.praetorius

merged

mentioned in commit 503c6f2a