Add some perfect forwarding in the Kernel

[sloriot]

No description provided.

[mglisse]

This seems to replace the copy in Cartesian_converter with a move, that's good. Getting rid of the move should be possible (instead of directly calling the converter on a double, use an object that has a conversion operator to the exact FT and lazily calls the converter in this operator), but probably not worth it.

[sloriot]

Successfully tested in CGAL-5.6-Ic-241. 2D version can be added too now.

[mglisse]

This seems to replace the copy in Cartesian_converter with a move, that's good. Getting rid of the move should be possible (instead of directly calling the converter on a double, use an object that has a conversion operator to the exact FT and lazily calls the converter in this operator), but probably not worth it.

Just a proof of concept hack

--- a/Cartesian_kernel/include/CGAL/Cartesian_converter.h
+++ b/Cartesian_kernel/include/CGAL/Cartesian_converter.h
@@ -83,6 +83,15 @@ struct Converting_visitor : boost::static_visitor<> {
 
 } // namespace internal
 
+template<class T2, class C, class T1> struct Marc {
+  CGAL_NO_UNIQUE_ADDRESS C c;
+  T1 const* in;
+  operator T2() const { return c(*in); }
+};
+template<class T2, class C, class T1> Marc<T2,C,T1> marc(C const&c, T1 const&x) {
+  return { c, &x };
+}
+
 template < class K1, class K2,
            class Converter /*= typename internal::Default_converter<K1, K2>::Type*/>
 class Cartesian_converter : public Enum_converter
@@ -292,7 +301,8 @@ public:
     operator()(const typename K1::Point_3 &a) const
     {
         typedef typename K2::Point_3 Point_3;
-        return Point_3(c(a.x()), c(a.y()), c(a.z()));
+        typedef typename K2::FT FT;
+        return Point_3(marc<FT>(c, a.x()), marc<FT>(c, a.y()), marc<FT>(c, a.z()));
     }
 
     typename K2::Weighted_point_3

When using Cartesian_converter on Point_3 from double to mpq_class, this removes some allocations. I expect it should also help with Gmpq, but not with boost types (I think they don't allocate on move).

(NewKernel_d uses a transform_iterator on the coordinates, for a similar effect)

[lrineau]

Several red lines in CGAL-6.0-Ic-227

/mnt/testsuite/include/CGAL/array.h:66:13: error: non-constant-expression cannot be narrowed from type 'int' to 'double' in initializer list [-Wc++11-narrowing]
   66 |   return {{ std::forward<Args>(args)... }};
      |             ^~~~~~~~~~~~~~~~~~~~~~~~

😢

[mglisse]

/mnt/testsuite/include/CGAL/array.h:66:13: error: non-constant-expression cannot be narrowed from type 'int' to 'double' in initializer list [-Wc++11-narrowing]
   66 |   return {{ std::forward<Args>(args)... }};
      |             ^~~~~~~~~~~~~~~~~~~~~~~~

#7373 (comment)

[lrineau]

@mglisse Can you please review ca1f11d and 940a7eb?

[afabri]

Compilation errors in CGAL-6.0-Ic-229

[lrineau]

@sloriot This PR can be tested again, after 15349f0. It should compile now with MSVC 2017 (but with that compiler the move/forwarding might be not optimal).

[afabri]

Compilation error 6.0-Ic-232 VC2017

[sloriot]

The error is gone in CGAL-6.0-Ic-233

[lrineau]

The error is gone in CGAL-6.0-Ic-233

Does it mean "fully tested"/"merge-able", or something else?

[sloriot]

It means that MSVC 2017 no longer crashes. It will be tested again tonight.

[sloriot]

Successfully tested in CGAL-6.0-Ic-235

[mglisse]

IIUC we use 2 overloads instead of perfect forwarding + make_array because make_array doesn't work well enough?