[WIP] Moving to C++ 14

[feliwir]

This implements the tasks mentioned in #1638 and #2202
The PR is currently still targeting C++ 17 until someone comes up with a replacement for shared_mutex, which was added in C++ 17.

Tasks:

• Replace boost thread dependency (thread, mutex, shared_mutex, condtion_variable)
• Use chrono for thread timing
• Update CMake to support C++ 14 (17)
• Use Lambdas instead of binds where appropiate (e.g. std::sort)
• Replace boost functional (bind, function)
• Replace boost random generators
• Replace boost date time
• Replace push_back with emplace_back (General replacement of push_back for emplace_back where appropriate. #2202)

Please check the changes for correctness

[taketwo]

Thanks for taking the initiative. I do understand your reasons for squashing everything into a single commit, however this is unmanageable for review. Too many changed files, too many changed lines, too many discussion points. So though this PR could serve as a starting point for discussions, I actually would strongly prefer to see a series of smaller PRs, each focusing on particular upgrade and each CI-tested.

Switching to 17 would be amazing, just constexpr if alone would simplify lots of things. But I think this would be too radical at this point in time. As far as I understood, your reason to propose this is to get rid of some Boost class. While it is a nice objective to replace Boost with std equivalents wherever possible, I don't think we have a big aim of ditching Boost altogether. PCL uses Boost meta-programming modules at its core, so that's not possible.

[feliwir]

Hey, thanks for your review. The reason for so many changes is that i can’t switch to e.g. std::thread and still use boost::posix_time for example. I know that it’s hard to review 700 files, but simply changing boost::shared_ptr to std::shared_ptr does modify >100 files. The purpose of removing boost::shared_mutex in favor of the std::variant was to remove the boost::thread dependency. There isn’t much use in switching to C++ 11 when we still had that.

On another note i don’t think it’s impossible to remove boost. Also with modern C++ there is no reason to use it in my opinion. Overly complicated boost template stuff can simply be simply changed into newer constructs e.g. Lambdas etc.

[taketwo]

The reason for so many changes is that i can’t switch to e.g. std::thread and still use boost::posix_time for example.

Then these two changes can go together. I did not mean to create a separate PR for every class. But things like shared_ptr are not related to threading, push_back/emplace_back conversion has no overlap with random generators, etc.

On another note i don’t think it’s impossible to remove boost.

Well, as I said we use Boost MPL and preprocessor libraries in PCL core (e.g. common/point_types.h or common/register_point_struct.h). If you can demonstrate how to replace those with standard C++14 features, please.

Also with modern C++ there is no reason to use it in my opinion

This is a very orthodox statement. Perhaps you should let the Boost developers/community know ;)

[feliwir]

Well the posix_time and std::thread stuff was just an example :) But yeah i could probably split the changes into more commits (or into more PRs?). I’ll redo that then.

As for boost MPL there is probably no 1 by 1 replacement. But maybe some similar stuff could be achieved with std::is_same/ std::tuple etc. But removing everything except MPL would already be a huge step forward.

[taketwo]

But yeah i could probably split the changes into more commits (or into more PRs?). I’ll redo that then.

I don't think we need separate PRs at this stage.

Regarding shared_ptr switch. We should probably be more careful with this change. Shared pointers are a part of our public interface. They are typically typedefed as Ptr and one may argue that whether a boost:: or a std:: shared pointer is used is an implementation detail. While this is true, I think it's not uncommon in user code to write things like boost::shared_ptr<PointCloud<pcl::PointXYZ>> directly. Another example is usage of boost::make_shared to create PCL point clouds. If we just convert all of our boost::shared_ptr to std::shared_ptr, lots of user code will get broken.

I was thinking along the lines of introducing pcl::shared_ptr and pcl::make_shared, which aliases to either Boost or stdlib, this being a build option. Default would be standard library, however the users that for one or another reason have to use Boost pointers will have an option to do that.

It could well be that I'm over-thinking. I'd like to hear other opinions on this.

[feliwir]

Well i don’t really see the need to support the boost smart pointers anymore. I think it would break very little user code, since everyone is already using the typedef.

If that’s not the case some changes are indeed necessary, but i think the vast majority of people is already using C++11 smart pointers anyway.

The move to C++14 will introduce major changes eitherways, so it‘s hard to avoid that the user must do some changes.

[taketwo]

Perhaps I need to clarify my position here. I believe that it's not okay to break user's code, unless it's absolutely unavoidable. And if so, then a reasonably easy way should be provided to write user code that is compatible with both PCL master and at least the last released version.

[feliwir]

@taketwo i was simply refering to the fact that this change is very unlikely to break any usercode. Noone is using boost smart pointers these days anymore. If someone encounters this issue it's dead simple to change boost:: into std:: (again i don't see anyone being affected by this).

Of course i don't want to break usercode on purpose aswell, but i simply don't see many cases where this would happen. It probably causes much more confusion that PCL pointers aren't compatible with C++ smart pointers.

[taketwo]

Noone is using boost smart pointers these days anymore.

For one, ROS still uses Boost smart pointers.

If someone encounters this issue it's dead simple to change boost:: into std::

No, it's not dead simple. Suppose you have a code base that depends on PCL and your target platform is 18.04. For deployment you install pre-compiled PCL from Debian, so you end up having 1.8.1. Now suppose that some of your developers want to experiment with new features and use PCL master on their machines. What do you do? How do you organize your code such that it's compatible both with 1.8.1 and master? (Note that I'm not even talking about ABI compatibility, only API.)

[feliwir]

Alright you got me convinced. I guess adding a macro that changes the Ptr typedef to boost smart pointers should be no issue. Do we want to default to boost smart pointers or to std smart pointers though?

[taketwo]

Do we want to default to boost smart pointers or to std smart pointers though?

I'd default to standard. We keep Boost pointers around only to make transition less painful for those who target multiple PCL versions.

It would be great to hear an opinion on this whole idea from more seasoned maintainers. @jspricke? @SergioRAgostinho your are also welcome to chime in!

[SergioRAgostinho]

Sorry for the late feedback (I'm a free man once more in two days).

Thanks for taking the initiative. I do understand your reasons for squashing everything into a single commit, however this is unmanageable for review. Too many changed files, too many changed lines, too many discussion points. So though this PR could serve as a starting point for discussions, I actually would strongly prefer to see a series of smaller PRs, each focusing on particular upgrade and each CI-tested

Idea: what about a separate branch to actually handle this transition? The master branch is kept clean of the intermediate state and we could start merging individual PRs. Downsides: we risk facing problems if someone submits a PR to master using Boost types which were already removed in the c++14 branch. These would always pop-up in the final PR from the c++14 branch to master one. Alternatively we can always ask to submit patches against the new branch .

Switching to 17 would be amazing, just constexpr if alone would simplify lots of things. But I think this would be too radical at this point in time. As far as I understood, your reason to propose this is to get rid of some Boost class. While it is a nice objective to replace Boost with std equivalents wherever possible, I don't think we have a big aim of ditching Boost altogether. PCL uses Boost meta-programming modules at its core, so that's not possible.

I would prefer to stick with the 14 revision for now. That's what the compilers are shipping by default and I feel that's what most libraries are aiming to support at this point.

@taketwo i was simply refering to the fact that this change is very unlikely to break any usercode. Noone is using boost smart pointers these days anymore. If someone encounters this issue it's dead simple to change boost:: into std:: (again i don't see anyone being affected by this).

I remember reading a comment from @v4hn somewhere regarding this effort of replacing the shared pointers from boost to std ended up being fairly more involved than anyone else was expecting, in some other project. I just don't remember if they were aiming for a pure replacement or actually supporting both by abstracting a call to the shared pointer. Pinging in hopes he knows what I'm referring to.

[feliwir]

required for CMAKE_CXX_STANDARD which allows us to set CXX Standard in a portable manner. As a sideeffect allows removing Policies, which are a default in this CMake version

[SergioRAgostinho]

Hey. We can now start merging C++14 stuff :)

Regarding the policies, it's ok to get rid of the ones which explicitly set the behavior to NEW but the ones setting to behavior toOLD need to be kept. We will address them on a separate PR, since they will require considerable changes.

[feliwir]

Idea: what about a separate branch to actually handle this transition? The master branch is kept clean of the intermediate state and we could start merging individual PRs. Downsides: we risk facing problems if someone submits a PR to master using Boost types which were already removed in the c++14 branch. These would always pop-up in the final PR from the c++14 branch to master one. Alternatively we can always ask to submit patches against the new branch .

I started to split this PR into smaller commits now if that is of any help. I'll comment on the changes so we can discuss the sideeffects, correct choices. Doing the transition on a seperate branch might be helpful, but i am not sure if this makes integration even harder in the end.

Also is there some test suite that i can run to check for the correctness of my changes?

[feliwir]

the timed_join function doesn't exist in standard C++ (and is also deprecated in boost). I didn't really understand that purpose of that check. Maybe someone can clarify this?

[feliwir]

interrupt() does not exist in standard C++, but this is also not required since there is an exit condition for the hdl_read_packet_thread_

[taketwo]

Idea: what about a separate branch to actually handle this transition?

We can do that. I'd say that if we get to a point where some of the transition code is mature enough to be merged before 1.9.0 is released, then we can start this branch.

Regarding the smart pointer hassle, there were big troubles when MoveIt converted to std pointers, but ROS did not. I'm not sure if this is the case you are referring to though.

@feliwir Yeah CMake is another big topic. a) We will need to decide on a new reasonable minimum version. b) Travis setup needs to be updated to use the new version.

Also is there some test suite that i can run to check for the correctness of my changes?

You can run make tests locally. But we also have CI integration on Travis (which your changes to CMake scripts broke, by the way).

[taketwo]

In fact, I think updating the CI setup should be the first task on the list. With that in place, all the subsequent changes can be tested automatically both on Linux and Windows.

[feliwir]

In fact, I think updating the CI setup should be the first task on the list. With that in place, all the subsequent changes can be tested automatically both on Linux and Windows.

I don't see how i did cause OpenNI to not be found. Are you sure that the commit that i've branched from was fully working?

[taketwo]

First, you can always check Travis build history: https://travis-ci.org/PointCloudLibrary/pcl/builds. It's green.

Second, OpenNI2 is just a warning, errors come later. Looking at them, I don't immediately get what's the problem. The fact that gcc's 4.8 standard library is used is suspicious though. But I would start by splitting the CMake changes into a separate commit and checking how that goes.

BTW what's your development environment?

[feliwir]

I am on Ubuntu 18.04, but i also have Windows with VS2017 available. The cmake stuff is in a seperate commit already. That the gcc version might be too old is a good point. I‘ll investigate tomorrow when i have time, thanks!

[v4hn]

I remember reading a comment from @v4hn somewhere regarding this effort of replacing the shared pointers from boost to std ended up being fairly more involved

Regarding the smart pointer hassle, there were big troubles when MoveIt converted to std pointers, but ROS did not. I'm not sure if this is the case you are referring to though.

The (main) pull-request for this effort can be found here: moveit/moveit#215
We wanted to make the code as agnostic as possible to whether boost or c++11 pointers are used.

Replacing shared_ptr by itself is not a big deal really, as stated here before.
The problems start when

• some modules used explicit boost::shared_ptr<...> instead of typedef'ed alternatives because these were partially not even available for some dependencies (urdfdom provided some only in newer releases).
• a class uses enable_shared_from_this
• weak_ptr is used, which is usually not typedef'ed
• ROS' pluginlib provided only boost pointers (not anymore)
• Ubuntu Xenial - the realistically most used distribution used with ROS these days - ships gcc 5.3.1 by default which does not default to C++11 yet. So every package that includes your library's headers has to specify the C++11/14 standard explicitly in its build instructions or will fail with "std::shared_ptr not defined". This is quite a hassle for users. Otoh it will not be a problem anymore with ROS melodic onwards because newer compilers default to C++11/14.

I guess that somewhat summarizes our experience with this in MoveIt.

Would be great to see C++14 in PCL in the next release!

[taketwo]

Thanks for the overview! I hope none of these problems will bite us, though some CMake magic will be needed to address the last one.

[v4hn]

though some CMake magic will be needed to address the last one.

We considered that too, but dismissed the idea. Libraries are not supposed to define the language standard, because client libraries might want to override them - use `gnu++ab` instead of `c++ab` or using a more current standard. Solutions that conditionally adds the flags are not robust and pollute cmake interfaces...

[taketwo]

Good point. Then at the very least we can put a macro in one of our include files that will check the standard and #error if it in less than C++14.

[v4hn]

put a macro in one of our include files that will check the standard and `#error` if it in less than C++14.

👍

[LeroyR]

Is it not possible to simply use clang-tidy modernize* checks to start the move towards c++14

[SergioRAgostinho]

Good point. Then at the very least we can put a macro in one of our include files that will check the standard and #error if it in less than C++14.

By the way @claudiofantacci brought this to my attention just two days ago, which surprising turned out to immediately relevant to this check. https://developercommunity.visualstudio.com/content/problem/120156/-cplusplus-macro-still-defined-as-pre-c11-value.html

[claudiofantacci]

By the way @claudiofantacci brought this to my attention just two days ago, which surprising turned out to immediately relevant to this check. https://developercommunity.visualstudio.com/content/problem/120156/-cplusplus-macro-still-defined-as-pre-c11-value.html

A very unfortunate situation having MS going against a C++ standard macro until VS version 15.7+.
Some libraries, like thrust, used the __cplusplus macro quite a lot that should not be use lightly for having portable code.

[stale]

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

[taketwo]

@SergioRAgostinho I suppose we can close this now since all the changes present in this PR have been (indirectly) merged.

[SergioRAgostinho]

Indeed. For anyone wishing to follow, please check the release issue #2772, we're keeping track of things there.

[feliwir]

Finally :)