Add CGAL_FOREACH by sloriot · Pull Request #2634 · CGAL/cgal

sloriot · 2017-11-27T17:44:02Z

Introduces CGAL_FOREACH that uses c++11 range loop for if available or BOOST_FOREACH.
There is a tiny variation as std::pair<T,T> is binded on the fly to Iterator_range<T>.

The files to be reviewed is STL_Extension/include/CGAL/foreach.h.

In passing 2bb0e40 and e34ca45 are bug-fixes not related to the change.

Fixes #2627
Replaces #2626

lrineau · 2017-11-28T11:12:59Z

STL_Extension/include/CGAL/foreach.h

+}
+
+} }
+


@mglisse Would you mind reviewing this file with the four overloads of CGAL::for_range_loop::make_range?

I am not sure but I think:

The three overloads about std::pair could be replaced by one taking its parameter by copy.

Maybe the fourth overload (generic) of make_range is not optimal.

The three overloads about std::pair could be replaced by one taking its parameter by copy.

Forgot to comment about this part but indeed, the last 2 std::pair overloads are redundant, the first one (const&) should suffice.

mglisse · 2017-11-28T13:13:58Z

STL_Extension/include/CGAL/foreach.h

+
+#define CGAL_FOREACH(A,B) for(A : CGAL::for_range_loop::make_range(B))
+#else
+#include <CGAL/foreach.h>


Did you mean to include a boost header instead here?

Right, I did not exclude the header from sed input files

mglisse · 2017-11-28T13:35:27Z

STL_Extension/include/CGAL/foreach.h

+#else
+/// \ingroup  PkgStlExtension
+/// If the version of `boost` is at least 1.51 and the compiler used support range-based for loops
+/// introduced with \cpp11, use it; otherwise fallback onto `CGAL_FOREACH`.


Overeager sed again?

mglisse · 2017-11-28T14:39:02Z

The generic version is dangerous. If I have a function f that returns a vector (by value), CGAL_FOREACH(whatever,f()) will destruct the vector before even calling begin(). It would be safer to have T make_range(T&&) and T& make_range(T&), if you keep a make_range.

mglisse · 2017-11-28T14:40:45Z

Special handling for std::pair is a hack to delay the inevitable: stop using std::pair as a range. I can understand not being in a hurry to fix it though.

lrineau · 2017-11-28T17:26:13Z

The generic version is dangerous. If I have a function f that returns a vector (by value), CGAL_FOREACH(whatever,f()) will destruct the vector before even calling begin(). It would be safer to have T make_range(T&&) and T& make_range(T&), if you keep a make_range.

What do you mean?

f() will be a temporary: a r-value of type vector<T>, so the generic make_range will be instantiated with vector<T>&&, and return a vector<T>&& as well. I do not see why you say the temporary will be destroyed before the call to begin(..).

mglisse · 2017-11-28T18:47:06Z

Try it on an example?
for(lhs:rhs) starts by doing auto&&range=rhs;. If rhs is a prvalue (a temporary object), range binds to it and extends its lifetime. If rhs is an xvalue (rvalue reference), range binds to the same object as the xvalue, but no lifetime extension occurs. The vector is destroyed. Next, for proceeds to initialize begin, end, and do the looping, on an object that is already dead.

lrineau · 2017-11-29T13:04:38Z

Travis detected an error:

/home/travis/build/CGAL/cgal/Arrangement_on_surface_2/test/Arrangement_on_surface_2/test_spherical_removal.cpp:88:16: error: 
      unexpected type name 'Halfedge_handle': expected expression
  CGAL_FOREACH(Halfedge_handle hh, arr.halfedge_handles())
               ^
/home/travis/build/CGAL/cgal/Arrangement_on_surface_2/test/Arrangement_on_surface_2/test_spherical_removal.cpp:111:45: error: 
      expected '(' for function-style cast or type construction
    CGAL_FOREACH(Arrangement_2::Face_handle fh, arr.face_handles())
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~ ^

sloriot · 2018-01-03T17:11:47Z

@mglisse I think my last commit fixes the issue you mentioned. Do you see a better solution? (I cannot remove the extra overloads for std::pair as it does not pass the testsuite. I'll investigate).

mglisse · 2018-01-03T17:29:49Z

I think my last commit fixes the issue you mentioned.

Yes.

Do you see a better solution?

Stop using std::pair as ranges so we don't need the make_range helper at all? Stop supporting C++03?

(I cannot remove the extra overloads for std::pair as it does not pass the testsuite. I'll investigate).

You mean removing the & and && versions, keeping const&? I am curious what might cause it to fail...

sloriot · 2018-01-04T08:43:50Z

Here is the problematic call:

CGAL_FOREACH(Cell_handle& ch,
  std::make_pair(cells.begin(), cells.end()))

mglisse · 2018-01-04T09:36:42Z

Ah yes, of course, once you have a fully generic make_range overload, it is more specialized than the const pair& overload for a pair rvalue. We could use sfinae to disable the generic version for pairs, or dispatch some other way, but I guess what you have now is simpler. Let's just hope that nobody writes a function returning a const pair, or you'll need the extra const&& overload...

By the way, could we start replacing those std::make_pair with boost::make_iterator_range? These are the easy cases where the use of std::pair is not baked in the API.

mglisse · 2018-01-04T11:37:56Z

replace std::make_pair with CGAL::make_range in foreach loops

thanks 👍

lrineau · 2018-01-04T14:48:26Z

conflicts

…ST_FOREACH this is to work around a bug observed with MSVC 2015

sloriot · 2018-01-04T15:18:29Z

Rebased

sloriot · 2018-01-05T09:22:39Z

There is the following error reported by travis with gcc 4.8.4.
I don't understand why it is ambiguous since get_chill() returns a lvalue reference.

/home/travis/build/CGAL/cgal/Classification/examples/Classification/../../include/CGAL/Classification/Sum_of_weighted_features_classifier.h: In member function ‘bool CGAL::Classification::Sum_of_weighted_features_classifier::load_configuration(std::istream&, bool)’:
/home/travis/build/CGAL/cgal/Classification/examples/Classification/../../include/CGAL/Classification/Sum_of_weighted_features_classifier.h:736:5: error: call of overloaded ‘make_range(boost::property_tree::basic_ptree<std::basic_string<char>, std::basic_string<char> >&)’ is ambiguous
     CGAL_FOREACH(boost::property_tree::ptree::value_type &v, tree.get_child("classification.features"))
     ^
/home/travis/build/CGAL/cgal/Classification/examples/Classification/../../include/CGAL/Classification/Sum_of_weighted_features_classifier.h:736:5: note: candidates are:
/usr/local/include/CGAL/foreach.h:55:14: note: constexpr T& CGAL::for_range_loop::make_range(T&) [with T = boost::property_tree::basic_ptree<std::basic_string<char>, std::basic_string<char> >]
 constexpr T& make_range(T& t)
              ^
/usr/local/include/CGAL/foreach.h:61:13: note: constexpr T CGAL::for_range_loop::make_range(T&&) [with T = boost::property_tree::basic_ptree<std::basic_string<char>, std::basic_string<char> >&]
 constexpr T make_range(T&& t)
             ^

mglisse · 2018-01-05T09:30:51Z

There is the following error reported by travis with gcc 4.8.4.

Looks like a bug in gcc-4.8, it compiles fine with 4.9.

lrineau · 2018-01-08T10:25:39Z

Travis shows a compilation issue: https://travis-ci.org/CGAL/cgal/jobs/325369172#L1550

What is more, CGAL-4.12-Ic-134 seems rather red on older Windows, because of the lack of support of constexpr.

lrineau · 2018-01-08T10:54:34Z

On my machine, /usr/include/boost/config/compiler/visualc.hpp contains:

// C++11 features supported by VC++ 14 update 3 (aka 2015)
//
#if (_MSC_FULL_VER < 190024210)
#  define BOOST_NO_CXX14_VARIABLE_TEMPLATES
#  define BOOST_NO_SFINAE_EXPR
#  define BOOST_NO_CXX11_CONSTEXPR
#endif

That means that constexpr was supported very late my MSVC: only in 2015 update 3. That explains why the code does not compile for versions of MSVC that know c++11 range-based for, but not constexpr.

lrineau · 2018-05-16T15:07:43Z

Should this PR be removed?

sloriot · 2018-05-16T15:35:09Z

well the issue is still here but I need to look again to see how to fix the remaining issues.

sloriot · 2019-03-15T12:05:30Z

No longer needed with the move to C++14

sloriot added Tests failing Cleaning and removed Tests failing labels Nov 28, 2017

sloriot mentioned this pull request Nov 28, 2017

PMP - fix remove_self_intersections() by removing BOOST_FOREACH #2626

Closed

lrineau reviewed Nov 28, 2017

View reviewed changes

mglisse reviewed Nov 28, 2017

View reviewed changes

lrineau added the Tests failing label Nov 29, 2017

sloriot force-pushed the CGAL-add_CGAL_FOREACH branch from 6f5bdd0 to 53f0303 Compare January 3, 2018 14:00

lrineau removed the Tests failing label Jan 4, 2018

sloriot added 9 commits January 4, 2018 16:16

add CGAL_FOREACH macro using either c++11 range-based for loop or BOO…

ac465fa

…ST_FOREACH this is to work around a bug observed with MSVC 2015

BOOST_FOREACH -> CGAL_FOREACH in header files

c57ace4

add missing \ingroup

7b42ece

document CGAL_FOREACH

03ad1bb

use CGAL_FOREACH in tests and examples

e51d015

update header that really needs to include boost/foreach.hpp

c6a6bde

return an Iterator_range instead of a pair

57d7b1a

add a workaround to make pair<T,T> iterable

2e70bad

add another overload for std::pair

c81e08e

sloriot added 3 commits January 4, 2018 16:18

add missing include directive

a1c0485

fix doc

e95ed67

use std::move for rvalues

07c3835

sloriot force-pushed the CGAL-add_CGAL_FOREACH branch from 9dccf63 to 07c3835 Compare January 4, 2018 15:18

lrineau added the Ready to be tested label Jan 4, 2018

lrineau added this to the 4.12-beta milestone Jan 4, 2018

lrineau self-assigned this Jan 4, 2018

lrineau added the Under Testing label Jan 4, 2018

we also need rvalue references

1dcb3f5

lrineau added Tests failing and removed Under Testing labels Jan 8, 2018

lrineau modified the milestones: 4.12-beta, 4.13-beta Jan 24, 2018

lrineau removed the Ready to be tested label May 16, 2018

lrineau added the Stalled label May 16, 2018

lrineau removed this from the 4.13-beta milestone Jun 26, 2018

lrineau added this to the Trash / Attic milestone Jan 7, 2019

sloriot closed this Mar 15, 2019

sloriot deleted the CGAL-add_CGAL_FOREACH branch May 3, 2019 06:27

MaelRL added Invalid and removed Tests failing Stalled labels Jan 15, 2020

Conversation

sloriot commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrineau Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

mglisse Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

mglisse Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

sloriot Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

mglisse Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

mglisse commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mglisse commented Nov 28, 2017

Uh oh!

lrineau commented Nov 28, 2017

Uh oh!

mglisse commented Nov 28, 2017

Uh oh!

lrineau commented Nov 29, 2017

Uh oh!

sloriot commented Jan 3, 2018

Uh oh!

mglisse commented Jan 3, 2018

Uh oh!

sloriot commented Jan 4, 2018

Uh oh!

mglisse commented Jan 4, 2018

Uh oh!

mglisse commented Jan 4, 2018

Uh oh!

lrineau commented Jan 4, 2018

Uh oh!

sloriot commented Jan 4, 2018

Uh oh!

sloriot commented Jan 5, 2018

Uh oh!

mglisse commented Jan 5, 2018

Uh oh!

lrineau commented Jan 8, 2018

Uh oh!

lrineau commented Jan 8, 2018

Uh oh!

lrineau commented May 16, 2018

Uh oh!

sloriot commented May 16, 2018

Uh oh!

sloriot commented Mar 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sloriot commented Nov 27, 2017 •

edited

Loading

mglisse commented Nov 28, 2017 •

edited

Loading