Skip to content

PMP - fix remove_self_intersections() by removing BOOST_FOREACH#2626

Closed
janetournois wants to merge 4 commits intoCGAL:masterfrom
janetournois:PMP-repair_fix_foreach-GF
Closed

PMP - fix remove_self_intersections() by removing BOOST_FOREACH#2626
janetournois wants to merge 4 commits intoCGAL:masterfrom
janetournois:PMP-repair_fix_foreach-GF

Conversation

@janetournois
Copy link
Copy Markdown
Member

@janetournois janetournois commented Nov 23, 2017 •
edited
Loading

Summary of Changes

In PMP/repair.h, we had to replace BOOST_FOREACH() with a c++11 for loop to avoid a seg fault.

For some reason, the modified Polyhedron gets corrupted when
BOOST_FOREACH() loops are used here, even though these loops are
not executed (because border_edges_found is false)

Note this crash only concerns visual studio 2015 (tested with visual 2013, clang, and g++), with -O2 and not -O1.
I am using boost 1.59

edit : Note also that I did not have to change all the BOOST_FOREACH in the file, changing those corresponding to code modifying the mesh is enough

Release Management

  • Affected package : PMP
  • Platform : visual 2015

@janetournois
add verbosity for debugging
6b3ea59
@janetournois
remove nested if, already tested
cf5af0b
@janetournois
replace BOOST_FOREACH with a c++11 for loop
0fe55d3 
for some reason, the modified Polyhedron gets corrupted when
BOOST_FOREACH loops are used here, even though these loops are
not executed (because border_edges_found is false)
@janetournois
fix implicit conversion warning (size_type to bool)
9b00be7
@janetournois janetournois requested a review from sloriot November 23, 2017 16:36
@lrineau
Copy link
Copy Markdown
Member

lrineau commented Nov 23, 2017

That is only for C++11 or later. As far as I know, PMP/repair.h is supposed to support all our supported compilers, including those using C++<11.

@janetournois janetournois mentioned this pull request Nov 23, 2017
Closed
@janetournois
Copy link
Copy Markdown
Member Author

janetournois commented Nov 23, 2017

Actually the issue #2627 suggests to add a CGAL_FOREACH() function that would use BOOST_FOREACH() in the general case, and the c++11 for loop for platforms that meet the issue.
Maybe we should do that first.
But for now, this code only available in the demo.

@lrineau lrineau added the TODO label Nov 23, 2017
lrineau referenced this pull request Nov 23, 2017
@afabri
small fixes
3ad8f65
lrineau
lrineau reviewed Nov 23, 2017
View reviewed changes
Polygon_mesh_processing/include/CGAL/Polygon_mesh_processing/repair.h
if (visited.insert(h).second)
{
one_halfedge_per_border.push_back(h);
BOOST_FOREACH(halfedge_descriptor hh, halfedges_around_face(h, tm))
Copy link
Copy Markdown
Member

@lrineau lrineau Nov 23, 2017

Choose a reason for hiding this comment

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

That BOOST_FOREACH is nested... Would it be sufficient to convert only that one?

@sloriot sloriot added the Not yet approved The feature or pull-request has not yet been approved. label Nov 28, 2017
@sloriot
Copy link
Copy Markdown
Member

sloriot commented Nov 28, 2017 •
edited
Loading

I suggest that we wait to see if #2634 makes it into master, in such a case we would simply ignored this PR.

@sloriot sloriot mentioned this pull request Nov 28, 2017
Closed
@lrineau
Copy link
Copy Markdown
Member

lrineau commented Nov 28, 2017

@janetournois: This PR modifies more that just the for-loops. See the diff. Is that wanted?

If it is, we cannot just ignore this PR in favor of #2634.

@janetournois
Copy link
Copy Markdown
Member Author

janetournois commented Nov 28, 2017

you are right, the only commit about for-loops is 0fe55d3.

This PR also fixes a warning and slighlty improves the code. I suggest that I wait for #2634 to be merged, and then make a new PR out of this one with the 3 other commits (of lower importance, btw)

@lrineau
Copy link
Copy Markdown
Member

lrineau commented Jan 4, 2018

Do we want to fix that bug only in master (CGAL-4.12)?

@sloriot
Copy link
Copy Markdown
Member

sloriot commented Jan 4, 2018

4.12 and as stated above #2634 might replace it

@lrineau
Copy link
Copy Markdown
Member

lrineau commented Jan 4, 2018

But that means that the bug in <CGAL/Polygon_mesh_processing/repair.h> will only be fixed in master, and not in release branches. Is that wanted?

@sloriot
Copy link
Copy Markdown
Member

sloriot commented Jan 4, 2018 •
edited
Loading

Yes, it is inside a function only used in the demo (not documented).

@lrineau
Copy link
Copy Markdown
Member

lrineau commented Jan 4, 2018

Replaced by #2634.

@lrineau lrineau closed this Jan 4, 2018
@sloriot sloriot mentioned this pull request Jan 5, 2018
Merged
@sloriot
Copy link
Copy Markdown
Member

sloriot commented Jan 5, 2018

#2711 contains the 3 commits that there not related to the for loops.

@MaelRL MaelRL removed the TODO label Feb 12, 2018
@janetournois janetournois deleted the PMP-repair_fix_foreach-GF branch October 25, 2022 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lrineau lrineau lrineau left review comments

@sloriot sloriot Awaiting requested review from sloriot

Assignees

No one assigned

Labels

Bug CGAL 3D demo Not yet approved The feature or pull-request has not yet been approved. Pkg::PMP Replaced

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@janetournois @lrineau @sloriot @MaelRL