search

CGAL / cgal

The public CGAL repository, see the README below
https://github.com/CGAL/cgal#readme
Other
4.96k stars 1.39k forks source link

Extending the point type of Simple_cartesian<"long long"> is broken #5700

Open efifogel opened 3 years ago

efifogel commented 3 years ago

Issue Details

This compilation error is the results of a combination of things. First, the code used to work ~2 years ago. I don't know when it broke. Secondly, on Windows "int", "long int", and "long long int" translate to 32, 32, and 64-bit integral types, respectively, while in Linux they translates to 32, 64, and 64-bit integral types, respectively, So, to assure getting a 64-bit integral type "long long" is used.

The (interesting) error message follows.

/home/efif/trees/cgal/cgal_master/Number_types/include/CGAL/NT_converter.h:33:21: error: call of overloaded ‘__gmp_expr(const long long int&)’ is ambiguous
         return NT2(a);
                     ^
In file included from /home/efif/trees/cgal/cgal_master/STL_Extension/include/CGAL/is_convertible.h:23:0,
                 from /home/efif/trees/cgal/cgal_master/Algebraic_foundations/include/CGAL/Rational_traits.h:23,
                 from /home/efif/trees/cgal/cgal_master/Number_types/include/CGAL/number_type_basic.h:40,
                 from /home/efif/trees/cgal/cgal_master/Kernel_23/include/CGAL/basic.h:28,
                 from /home/efif/trees/cgal/cgal_master/Cartesian_kernel/include/CGAL/Cartesian/Cartesian_base.h:20,
                 from /home/efif/trees/cgal/cgal_master/Cartesian_kernel/include/CGAL/Cartesian.h:20,
                 from /home/efif/tmp/cgal/issues/long_gmp.cpp:1:
/usr/include/gmpxx.h:1661:3: note: candidate: __gmp_expr<__mpq_struct [1], __mpq_struct [1]>::__gmp_expr(double)
   __GMPXX_DEFINE_ARITHMETIC_CONSTRUCTORS
   ^
/usr/include/gmpxx.h:1661:3: note: candidate: __gmp_expr<__mpq_struct [1], __mpq_struct [1]>::__gmp_expr(float)
   __GMPXX_DEFINE_ARITHMETIC_CONSTRUCTORS
   ^
/usr/include/gmpxx.h:1661:3: note: candidate: __gmp_expr<__mpq_struct [1], __mpq_struct [1]>::__gmp_expr(long unsigned int)
   __GMPXX_DEFINE_ARITHMETIC_CONSTRUCTORS
   ^
/usr/include/gmpxx.h:1661:3: note: candidate: __gmp_expr<__mpq_struct [1], __mpq_struct [1]>::__gmp_expr(long int)
   __GMPXX_DEFINE_ARITHMETIC_CONSTRUCTORS
   ^
/usr/include/gmpxx.h:1661:3: note: candidate: __gmp_expr<__mpq_struct [1], __mpq_struct [1]>::__gmp_expr(short unsigned int)
   __GMPXX_DEFINE_ARITHMETIC_CONSTRUCTORS
   ^
/usr/include/gmpxx.h:1661:3: note: candidate: __gmp_expr<__mpq_struct [1], __mpq_struct [1]>::__gmp_expr(short int)
   __GMPXX_DEFINE_ARITHMETIC_CONSTRUCTORS
   ^
/usr/include/gmpxx.h:1661:3: note: candidate: __gmp_expr<__mpq_struct [1], __mpq_struct [1]>::__gmp_expr(unsigned int)
   __GMPXX_DEFINE_ARITHMETIC_CONSTRUCTORS
   ^
/usr/include/gmpxx.h:1661:3: note: candidate: __gmp_expr<__mpq_struct [1], __mpq_struct [1]>::__gmp_expr(int)
   __GMPXX_DEFINE_ARITHMETIC_CONSTRUCTORS
   ^
/usr/include/gmpxx.h:1661:3: note: candidate: __gmp_expr<__mpq_struct [1], __mpq_struct [1]>::__gmp_expr(unsigned char)
   __GMPXX_DEFINE_ARITHMETIC_CONSTRUCTORS
   ^
/usr/include/gmpxx.h:1661:3: note: candidate: __gmp_expr<__mpq_struct [1], __mpq_struct [1]>::__gmp_expr(signed char)
   __GMPXX_DEFINE_ARITHMETIC_CONSTRUCTORS
   ^
/usr/include/gmpxx.h:1648:3: note: candidate: __gmp_expr<__mpq_struct [1], __mpq_struct [1]>::__gmp_expr(__gmp_expr<__mpq_struct [1], __mpq_struct [1]>&&)
   __gmp_expr(__gmp_expr &&q)
   ^
/usr/include/gmpxx.h:1642:3: note: candidate: __gmp_expr<__mpq_struct [1], __mpq_struct [1]>::__gmp_expr(const __gmp_expr<__mpq_struct [1], __mpq_struct [1]>&)
   __gmp_expr(const __gmp_expr &q)
   ^

Source Code

#include <CGAL/Cartesian.h>
#include <CGAL/Simple_cartesian.h>
#include <CGAL/Filtered_kernel.h>

template<typename Kernel> struct Point_data {};

template <typename Kernel_, typename KernelBase>
class My_cartesian_base : public KernelBase::template Base<Kernel_>::Type {
public:
  typedef Kernel_                                                  Kernel;
  typedef KernelBase                                               Kernel_base;

  typedef typename Kernel_base::template Base<Kernel>::Type        Old_kernel;
  typedef typename Old_kernel::Point_2                             Old_point_2;
  typedef typename Old_kernel::RT                                  RT;

  /*! The new point type */
  class New_point_2 : public Old_point_2 {

  private:
    typedef boost::shared_ptr<Point_data<Kernel>>  Shared_point_data;
    Shared_point_data m_data;
  public:
    // Constructors
    New_point_2() {}
    New_point_2(CGAL::Origin origin) : Old_point_2(origin) {}
    New_point_2(const RT& x, const RT& y) : Old_point_2(x, y) {}
    New_point_2(const RT& x, const RT& y, const RT& w) : Old_point_2(x, y, w) {}

    // Getters & setters
    void set_data(Shared_point_data data) { m_data = data; }
    Shared_point_data data() const { return m_data; }
    size_t index() const { return m_data->index(); }

    // Operators
    bool operator==(const New_point_2& p) const
    { return Old_point_2(*this) == Old_point_2(p); }

    bool operator!=(const New_point_2& p) const { return !(*this == p); }
  };

  template <typename KernelT, typename OldKernel>
  class New_construct_point_2 {
    typedef KernelT                     Kernel;

    typedef typename Kernel::RT         RT;
    typedef typename Kernel::Point_2    Point_2;
    typedef typename Point_2::Rep       Rep;
  public:
    typedef Point_2                     result_type;

    // Note : the CGAL::Return_base_tag is really internal CGAL stuff.
    // Unfortunately it is needed for optimizing away copy-constructions,
    // due to current lack of delegating constructors in the C++ standard.
    Rep operator()(CGAL::Return_base_tag) const { return Rep(); }
    Rep operator()(CGAL::Return_base_tag, CGAL::Origin origin) const
    { return Rep(origin); }
    Rep operator()(CGAL::Return_base_tag, const RT& x, const RT& y) const
    { return Rep(x, y); }
    Rep operator()(CGAL::Return_base_tag, const RT& x, const RT& y, const RT& w)
      const
    { return Rep(x, y, w); }

    Point_2 operator()() const { return New_point_2(); }
    Point_2 operator()(CGAL::Origin origin) const
    { return New_point_2(origin); }
    Point_2 operator() (const Point_2& p) const {
      auto np = New_point_2(p);
      np.set_data(p.data());
      return New_point_2(np);
    }
    Point_2 operator()(const RT& x, const RT& y) const
    { return New_point_2(x, y); }
    Point_2 operator()(const RT& x, const RT& y, const RT& w) const
    { return New_point_2(x, y, w); }
  };

public:
  typedef New_point_2                                   Point_2;
  typedef New_construct_point_2<Kernel, Old_kernel>     Construct_point_2;

  Construct_point_2 construct_point_2_object() const
  { return Construct_point_2(); }

  template <typename KernelT>
  struct Base { typedef My_cartesian_base<KernelT, Kernel_base>  Type; };
};

template <typename FT_>
struct Extended_filtered_kernel : public CGAL::Filtered_kernel_adaptor<
  CGAL::Type_equality_wrapper<
    My_cartesian_base<Extended_filtered_kernel<FT_>,
                      CGAL::Simple_cartesian<FT_> >,
    Extended_filtered_kernel<FT_> >, true >
{};

int main() {
  std::cout << "sizeof(long): " << sizeof(long) << std::endl;
  std::cout << "sizeof(long long): " << sizeof(long long) << std::endl;
#if 0
  // typedef CGAL::Cartesian<long>         Kernel;
  typedef Extended_filtered_kernel<long>  Kernel;
#else
  // typedef CGAL::Simple_cartesian<long>         Kernel;
  typedef Extended_filtered_kernel<long long>  Kernel;
#endif
  Kernel kernel;
  typename Kernel::Point_2 p, q;
  auto res = kernel.equal_2_object()(p, q);
  return 0;
}

Environment

mglisse commented 3 years ago

GMPXX does not support long long. You can disable the use of GMPXX. Something like -DWITH_GMPXX=false for CMake? Or #undef CGAL_USE_GMPXX in your file. If it then fails because of boost, you may also need #define CGAL_DO_NOT_USE_BOOST_MP 1, and you should be back to good old Gmpq. int64_t is the standard way to get a 64-bit signed integer type.

efifogel commented 3 years ago

What the connection? I don't want to disable it. I'm actually using it. Anyway, as I've said, it used to work, and now it doesn't.

On Tue, May 11, 2021, 20:53 Marc Glisse @.***> wrote:

GMPXX does not support long long. You can disable the use of GMPXX through CMake (something like -DWITH_GMPXX=false?).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/CGAL/cgal/issues/5700#issuecomment-838908949, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVBNOFKDUUQPFX4ZK5RBELTNFVK5ANCNFSM44R3ZBPA .

mglisse commented 3 years ago

Filtered_kernel works by converting your objects to those in Simple_cartesian<Interval_nt> and Simple_cartesian<Exact_rational>. Exact_rational used to be a typedef for Gmpq. Now, by default, it is mpq_class or mpq_rational. Disabling those 2 "new" types makes it use Gmpq again, as it used to.

efifogel commented 3 years ago

Absolutely not. My (real) program uses gmpq and gmpz. I cannot disable them. As I've said, it used to work the way it is.

//) o /__ // (__ ( ( ( (/ (/-(-'(/ /

On Wed, 12 May 2021 at 07:02, Marc Glisse @.***> wrote:

Filtered_kernel works by converting your objects to those in Simple_cartesian and Simple_cartesian. Exact_rational used to be a typedef for Gmpq. Now, by default, it is mpq_class or mpq_rational. Disabling those 2 "new" types makes it use Gmpq again, as it used to.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/CGAL/cgal/issues/5700#issuecomment-839414995, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVBNOFIAPYNHSR5F6J4OLTTNH4UNANCNFSM44R3ZBPA .

mglisse commented 3 years ago

Will you please read my message? I am talking of making Gmpq/Gmpz the default by disabling other types. If you are also using GMPXX (not just GMP) or Boost.Multiprecision directly, then disabling them could be a problem, but it doesn't seem to be the case from what you are saying. Using int64_t instead of long long would help on 64-bit linux, and on windows using the MPIR fork of GMP might also help. I am not saying the current situation is perfect, just trying to suggest some workarounds.

efifogel commented 3 years ago

ok, you are trying to help suggesting a workaround and I've dismissed you---sorry. It turns out that the code compiles on Windows (MSVC), so there is another workaround: On Windows , where "long long" is mandatory it works, and on Linux, where "long" and "long long" are equivalent, one can use (single) "long". Nevertheless, I would still like it to be fixed, because it's code that we are about to publish (in a published paper).

//) o /__ // (__ ( ( ( (/ (/-(-'(/ /

On Wed, 12 May 2021 at 10:40, Marc Glisse @.***> wrote:

Will you please read my message? I am talking of making Gmpq/Gmpz the default by disabling other types. If you are also using GMPXX (not just GMP) or Boost.Multiprecision directly, then disabling them could be a problem, but it doesn't seem to be the case from what you are saying. Using int64_t instead of long long would help on 64-bit linux, and on windows using the MPIR fork of GMP might also help. I am not saying the current situation is perfect, just trying to suggest some workarounds.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/CGAL/cgal/issues/5700#issuecomment-839540826, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVBNOFSUVLLQF2FMBM3NRTTNIWFLANCNFSM44R3ZBPA .

mglisse commented 3 years ago

It turns out that the code compiles on Windows (MSVC),

Probably because you have installed MPIR and not GMP.

so there is another workaround: On Windows , where "long long" is mandatory it works, and on Linux, where "long" and "long long" are equivalent, one can use (single) "long".

int64_t conveniently does that.