Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Arr_observer<Arrangement_2> #7927

Merged
merged 10 commits into from
Jan 24, 2024
Merged

Conversation

efifogel
Copy link
Member

Summary of Changes

  1. Renamed the template Arr_observer to Aos_observer
  2. Added the nested type 'Observer' in the Aos_2 class template, which is an alias to Arr_observer<Aos_2>.
  3. Finally, introduced Arr_observer<Arrangement_2>, which is an alias to Arr_observer<Aos_2>, where Arrangement_2 derives from Aos_2 (for cases where Arrangement_2 is not Aos_2).

The first 2 items eliminate the error-prone c-type casting that was used to define observers.
The 3rd item is for backward compatibility.

Release Management

  • Affected package(s): Arrangement_on_surface_2, Minkowski_sum_2, Visibility_2, and Envelope_3
  • Issue(s) solved (if any): fix Pointer Authentification issues in Arrangement #7901
  • Feature/Small Feature (if any):
  • Link to compiled documentation (obligatory for small feature) wrong link name to be changed
  • License and copyright ownership:

…curs when using observers; renamed Arr_observer => Aos_observer, and introduced Arr_observer for backward compatibility.
*
* `Aos_observer` serves as an abstract base class for all observer classes that
* are attached to an arrangement instance of type `Arrangement` and receive
* notifications from the arrangement. This base class handles the attachment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 spaces should be 1 space (also on other places)

/*! the \f$ x\f$-monotone curve type. */
typedef typename Arrangement_2::X_monotone_curve_2 X_monotone_curve_2;

/*! */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be filled in? (also on the next few lines?)

Vertex_handle v1, Vertex_handle v2);

/*! issued immediately after a new edge `e` has been created. The halfedge that
* is sent to this function is always directed from `v1` to `v2` (see above).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better refer to the function name as e.g. due to sorting the order in the documentation can change.

#include <CGAL/Arr_enums.h>

/*! \file
* Definition of the Aos_observer<Arrangement> base class.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backticks around Aos_observer<Arrangement>?

/// \name Modifying the associated arrangement.
//@{

/*! Get the associated arrangement (non-const version). */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"non-const" version?

/*!
* Notification before the creation of a new vertex.
* \param p The point to be associated with the vertex.
* This point cannot lies on the surface boundaries.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"lies" -> "lie"

{}

/*! Notification before the creation of a new boundary vertex.
* \param p The on the surface boundary.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The on" ?

virtual void after_create_boundary_vertex(Vertex_handle /* v */) {}

/*! Notification before the creation of a new edge.
* \param c The x-monotone curve to be associated with the edge.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x-monotone elsewhere it is like a formula \f$ x\f$-monotone (also at other places)

Comment on lines +222 to +223
* \param v1 A handle to the first end-vertex of the edge.
* \param v2 A handle to the second end-vertex of the edge.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 end-vertices?

Comment on lines 228 to 229
/*! issued just before outer ccb `h` inside a face `f` is split into two, as a
* result of the removal of the edge `e` from the arrangement.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ccb or CCB (just above and in another file I saw CCB)

@lrineau lrineau changed the title Aos 2 observer efif Introduce Arr_observer<Arrangement_2> Dec 12, 2023
@lrineau lrineau added this to the 6.0-beta milestone Dec 12, 2023
@afabri
Copy link
Member

afabri commented Dec 13, 2023

Error in the documentation : CGAL-6.0-Ic-127/doc/scripts/doc_1_8_13/doc_tags/Arrangement_on_surface_2.tag:11511: warning: Duplicate anchor arr_refarr_obs found

@efifogel
Copy link
Member Author

efifogel commented Dec 13, 2023 via email

@efifogel
Copy link
Member Author

efifogel commented Dec 13, 2023 via email

@albert-github
Copy link
Contributor

Regarding the documentation warning when running doxygen 1.8.13

  • this might be a bug in the doxygen 1.8.13 version
  • is thus pull request just for the 6.0 and newer versions or will it also be in the 5.5 / 5.6 CGAL versions
    • in case it is only intended for the 6.0 and newer versions there won't be a problem as the doxygen 1.9.6 version will be used here (to the best of my knowledge)
    • in case it is also back-ported to 5.5 and 5.6 we have to see what the effect on the documentation is

@efifogel
Copy link
Member Author

efifogel commented Dec 13, 2023 via email

@sloriot
Copy link
Member

sloriot commented Dec 14, 2023

New warnings:

/mnt/testsuite/include/CGAL/Arr_trapezoid_ric_point_location.h:241:16: warning: 'CGAL::Arr_trapezoid_ric_point_location<CGAL::Arrangement_2<CGAL::Arr_segment_traits_2<>>>::before_assign' hides overloaded virtual function [-Woverloaded-virtual]
  241 |   virtual void before_assign (const Arrangement_on_surface_2& arr)
      |                ^
/mnt/testsuite/test/Arrangement_on_surface_2/test_vert_ray_shoot_vert_segments.cpp:60:10: note: in instantiation of template class 'CGAL::Arr_trapezoid_ric_point_location<CGAL::Arrangement_2<CGAL::Arr_segment_traits_2<>>>' requested here
   60 |   RIC_pl ric_pl(arr);
      |          ^
/mnt/testsuite/include/CGAL/Aos_observer.h:139:16: note: hidden overloaded virtual function 'CGAL::Aos_observer<CGAL::Arrangement_on_surface_2<CGAL::Arr_segment_traits_2<>, CGAL::Arr_bounded_planar_topology_traits_2<CGAL::Arr_segment_traits_2<>>>>::before_assign' declared here: type mismatch at 1st parameter ('const Arrangement_2 &' (aka 'const CGAL::Arrangement_on_surface_2<CGAL::Arr_segment_traits_2<>, CGAL::Arr_bounded_planar_topology_traits_2<CGAL::Arr_segment_traits_2<>>> &') vs 'const Arrangement_on_surface_2 &' (aka 'const CGAL::Arrangement_2<CGAL::Arr_segment_traits_2<>> &'))
  139 |   virtual void before_assign(const Arrangement_2& /* arr */) {}
      |                ^
In file included from /mnt/testsuite/test/Arrangement_on_surface_2/test_vert_ray_shoot_vert_segments.cpp:6:
/mnt/testsuite/include/CGAL/Arr_trapezoid_ric_point_location.h:263:16: warning: 'CGAL::Arr_trapezoid_ric_point_location<CGAL::Arrangement_2<CGAL::Arr_segment_traits_2<>>>::before_attach' hides overloaded virtual function [-Woverloaded-virtual]
  263 |   virtual void before_attach (const Arrangement_on_surface_2& arr)

@sloriot
Copy link
Member

sloriot commented Dec 14, 2023

…arr) of all observers (derived from Aos_observer)
@efifogel
Copy link
Member Author

efifogel commented Dec 14, 2023 via email

@efifogel
Copy link
Member Author

efifogel commented Dec 14, 2023 via email

@lrineau
Copy link
Member

lrineau commented Dec 19, 2023

New warnings:

/mnt/testsuite/include/CGAL/Arr_trapezoid_ric_point_location.h:241:16: warning: 'CGAL::Arr_trapezoid_ric_point_location<CGAL::Arrangement_2<CGAL::Arr_segment_traits_2<>>>::before_assign' hides overloaded virtual function [-Woverloaded-virtual]
  241 |   virtual void before_assign (const Arrangement_on_surface_2& arr)
      |                ^

Now that we use C++11 or later, prefer the use of override instead of virtual. That way all compilers will emit an error if a member function supposed to override a virtual function of a base class have a mismatch in the parameters. The specifier virtual is only for the base function of the hierarchy.

@sloriot
Copy link
Member

sloriot commented Dec 19, 2023

New warnings:

  • /Users/magritte/cgal_root/CGAL-6.0-Ic-131/include/CGAL/Arr_trapezoid_ric_point_location.h:301:16: warning: 'CGAL::Arr_trapezoid_ric_point_location<CGAL::Arrangement_with_history_2<CGAL::Arr_segment_traits_2<>>>::before_split_edge' hides overloaded virtual function [-Woverloaded-virtual] virtual void before_split_edge (Halfedge_handle e,

  • /Users/magritte/cgal_root/CGAL-6.0-Ic-131/include/CGAL/Arr_trapezoid_ric_point_location.h:325:16: warning: 'CGAL::Arr_trapezoid_ric_point_location<CGAL::Arrangement_with_history_2<CGAL::Arr_segment_traits_2<>>>::before_merge_edge' hides overloaded virtual function [-Woverloaded-virtual] virtual void before_merge_edge (Halfedge_handle e1,

Sorry, something went wrong.

@efifogel
Copy link
Member Author

efifogel commented Dec 20, 2023 via email

@efifogel
Copy link
Member Author

efifogel commented Dec 20, 2023 via email

@sloriot
Copy link
Member

sloriot commented Dec 27, 2023

New warnings:

  • /Users/magritte/cgal_root/CGAL-6.0-Ic-137/test/Arrangement_on_surface_2/test_observer.cpp:406:16: warning: 'after_merge_outer_ccb' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
  • /Users/magritte/cgal_root/CGAL-6.0-Ic-137/include/CGAL/Aos_observer.h:451:16: note: overridden virtual function is here virtual void after_merge_outer_ccb(Face_handle /* f */,

@efifogel
Copy link
Member Author

efifogel commented Dec 27, 2023 via email

@sloriot
Copy link
Member

sloriot commented Jan 24, 2024

Tests are looking OK in CGAL-6.0-Ic-156 but we are still having a segfault on Arm64e here

I should have a look locally.

@sloriot
Copy link
Member

sloriot commented Jan 24, 2024

The issue is not specific to macOs but requires to disable GMP. The branch can be merged I'll handle the issue in another branch.

@efifogel
Copy link
Member Author

efifogel commented Jan 24, 2024 via email

@sloriot
Copy link
Member

sloriot commented Jan 24, 2024

The issue comes AFAIU from the reading of input into Quotient<MP_Float>. If you configure the test with -DCGAL_DISABLE_GMP=ON you should be able to reproduce it on your machine.

@efifogel
Copy link
Member Author

efifogel commented Jan 24, 2024 via email

@sloriot
Copy link
Member

sloriot commented Jan 24, 2024

It's a bug in Quotient.h, I'll fix it.

@lrineau lrineau added the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Jan 24, 2024
@lrineau lrineau merged commit c0b02c4 into CGAL:master Jan 24, 2024
9 checks passed
@lrineau lrineau removed the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Jan 24, 2024
@sloriot sloriot deleted the Aos_2-observer-efif branch October 14, 2024 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pointer Authentification issues in Arrangement
5 participants