Skip to content

Triangulation: filter_iterator #8899

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

afabri
Copy link
Member

@afabri afabri commented May 21, 2025

Summary of Changes

The testsuite of Triangulation has N's for some platforms. The test platforms have in common to use boost_1_88_0, but with VC++ or clang on Windows I can't reproduce the error with this version of boost.

In this PR I use the filter_iterator adaptor of CGAL, to see if the boost filter_iterator is the problem.

Release Management

  • Affected package(s): Triangulation
  • License and copyright ownership: unchanged

Finite_full_cell_const_iterator finite_full_cells_begin() const
{ return Finite_full_cell_const_iterator(Finiteness_predicate(*this), full_cells_begin(), full_cells_end()); }
Finite_full_cell_const_iterator finite_full_cells_end() const
{ return Finite_full_cell_const_iterator(Finiteness_predicate(*this), full_cells_end(), full_cells_end()); }
#else
/*
Copy link
Member Author

Choose a reason for hiding this comment

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

@sloriot When I uncomment the non-const version I get a compilation error. Can you explain why?

Copy link
Member

Choose a reason for hiding this comment

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

Fixed. I don't get why I need to define extra operators. I would have expect that there would be an implicit conversion then.

@mglisse
Copy link
Member

mglisse commented May 24, 2025

See boostorg/iterator#92 for the source of the issue.

@afabri
Copy link
Member Author

afabri commented May 29, 2025

@mglisse I only changed the filter for cells and not for vertices, which is enough for the testsuite. What is different for vertices? Is it maybe only not tested?

@mglisse
Copy link
Member

mglisse commented May 29, 2025

It is the copy constructor (or conversion) of filter_iterator that is broken, in a way that mostly matters if you do ++ on a copy and the last element of the sequence does not satisfy the predicate but some earlier elements do (maybe also if you compare a copy to a non-copy, I didn't check). That's quite specific, so it is not surprising that tests don't always detect it.
If the infinite vertex is always the first vertex, then it seems plausible that Finite_vertex_iterator may be immune to the bug (but if that's the case, filter_iterator is overkill and there is a much easier way to implement Finite_vertex_iterator). If not, it is probably possible to trigger the bug for vertices as well.

@MaelRL MaelRL added this to the 6.1-beta milestone Jun 2, 2025
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.

4 participants