CGAL / cgal

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

STL_extension: regroup and populate CGAL/Functional.h #2993

Closed MaelRL closed 6 years ago

MaelRL commented 6 years ago

Issue Details

The CGAL boost/std picking shenanigans for <functional> (e.g. function) as well as forked deprecated std tools (e.g. unary_function) are split between multiple files in the package STL_extension. They should be merged and other useful tools such as ref should also be added.

Edit: note https://github.com/CGAL/cgal/pull/2985#issuecomment-381060798

afabri commented 6 years ago

We also have a file STL_extension/include/CGAL/function.h

MaelRL commented 6 years ago

Looking at it a bit more in detail, I have some doubts about this CGAL's <functional>. More precisely, what would be the point of CGAL::ref or others? As far as I understand, we have CGAL::unordered_whatever and CGAL::array because the cpp11 versions are better and so it makes sense to use them if they're available. We also have CGAL::unary/binary_function because they get deprecated and removed.

However, what would be the benefit of using std::ref over boost::ref as these are neither deprecated nor removed (and similarly, why is there CGAL::function?). Is there really some significant compilation/run time differences?

Worse, boost::bind and std::bind have apparently (see this SO question) some significant differences so it seems a rather bad idea to have CGAL::bind. (I don't know if it is the same for ref/cref and others.)

lrineau commented 6 years ago

Why not a <CGAL/functional.h> with only the tools we need?

MaelRL commented 6 years ago

bind, ref, function, cref, etc. are all used across CGAL, but I don't see any reason to put them in a CGAL namespace and to switch to the std:: version if available rather than just use boost:: all the time. Except if the std:: version is always much better, which I am not sure about (especially for stuff like bind), thus my question(s) above.

lrineau commented 6 years ago

I agree. I would put in that file <CGAL/functional.h> only the functions and class templates for which we have an implementation.

MaelRL commented 6 years ago

So basically only unary/binary_function, which is already in that file. Thus, the only change we really want to make is put those in a cpp98 namespace.

Should I remove function.h and simply use boost::function with symmetry with bind, etc. in mind? (I have permission from @sgiraudot !)

lrineau commented 6 years ago

@MaelRL commented on Jun 12, 2018, 2:51 PM GMT+2:

So basically only unary/binary_function, which is already in that file. Thus, the only change we really want to make is put those in a cpp98 namespace.

Should I remove function.h and simply use boost::function with symmetry with bind, etc. in mind? (I have permission from @sgiraudot !)

In my opinion that goes the wrong way: we should try to use more of std:: and less of Boost.

Either we merge <CGAL/function.h> into CGAL/functional.h, or we do nothing. Anyway, we cannot remove <CGAL/function.h> completely, because it is documented. At least it should remain as a compatibility header including <CGAL/functional.h>. Why not do nothing and close this issue?

MaelRL commented 6 years ago

Dunno, the point was to order a bit stuff, but we can also do nothing.

Do I also drop the cpp98 namespace changes for unary/binary_function, iterator, (and possibly others, I don't know)?

lrineau commented 6 years ago

If you already have a branch for that, let's use it. In my opinion, the CGAL::cpp98 namespace should be used for things that were deprecated, but that we duplicated for convenience (iterator, binary_function, etc.).

MaelRL commented 6 years ago

Done in #3170.

Closing that, conclusion here is to leave function.h alone and there is (currently) no point adding further switch mechanisms for .