Using std::optional for Property_container::get<T>

[soesau]

Summary of Changes

Switching from std::pair<Property_map<T>, bool> to std::optional in Property_container::get<T>

Introducing Pair_optional_adaptor for backward compatibility which extends std::optional<T> to interface of std::pair

using Pair_optional_adaptor for Surface_mesh and Point_set_3

Release Management

• Affected package(s): Point_set_3, Surface_mesh, STL_Extension

[lrineau]

This class Pair_optional_adaptor<T> has four data members:

• its base class, of type std::optional<T>,
• T &first and bool second for the compatibility with std::pair<T, bool>,
• and t_storage (I am not sure why it is an union instead of a plain T).

That makes that class source-incompatible with:

auto [pmap, ok] = point.get<Foo>("bar");

See live on compiler explorer: https://godbolt.org/z/3xY6qzPTq

[lrineau]

I am in favor of a real breaking change:

• change the API of CGAL::Property_container (undocumented anyway),

• In Surface_mesh.h, add:

  • vertex_property_map,
  • halfedge_property_map,
  • edge_property_map, and
  • face_property_map,

  with the new API, and keep the old property_map,

• a breaking change in Point_set_3.h.

[soesau]

* and `t_storage` (I am not sure why it is an union instead of a plain `T`).

I used the union originally to avoid construction of T in case there is no default constructor. Sebastien mentioned already that T will be some lightweight object anyway that has to be default constructible for being put into a pair.

The PR is still WIP. I made it work locally for Surface_mesh and Point_set_3 and then created the PR to see if this works in the CI.

So, I'll remove the union and switch to a plain T.

[sloriot]

We decided during meeting to break the compatibility and use std::optional directly.

[sloriot]

@soesau Note that this PR is a release blocker and it should be updated following the discussion we had with @lrineau and @afabri

[lrineau]

@sloriot, in a message dated Feb 2024:

We decided during meeting to break the compatibility and use std::optional directly.

@sloriot, yesterday:

@soesau Note that this PR is a release blocker and it should be updated following the discussion we had with @lrineau and @afabri

@sloriot, why is it release blocker?

@soesau If this PR is really blocking the next release, then we should hurry. Do you need help on it?

[sloriot]

The API change is possible thanks to std::optional that is introduced in CGAL APIs with CGAL 6.0, and Orthtree is already using it for its dynamic property system.

[soesau]

This class Pair_optional_adaptor<T> has four data members:

• its base class, of type std::optional<T>,
• T &first and bool second for the compatibility with std::pair<T, bool>,
• and t_storage (I am not sure why it is an union instead of a plain T).

That makes that class source-incompatible with:

auto [pmap, ok] = point.get<Foo>("bar");

See live on compiler explorer: https://godbolt.org/z/3xY6qzPTq

We decided for a breaking change, thus there will be no adaptor.

[sloriot]

Successfully tested in CGAL-6.0-Ic-245