Migrate boost::function to std::function

[SergioRAgostinho]

This one requires some discussion. There's blatant API breakage, but I'm not really keen in keeping functions with signatures including boost types, around. I need more opinions on how to address this.

[taketwo]

I guess this can be counted as a "soft" API breakage. The changed types are either hidden behind typedefs or are compatible. In most cases the existing user code should still compile, right?

[SergioRAgostinho]

In most cases the existing user code should still compile, right?

This is the situation that I'm foreseeing to be a problem. User code of this type

// make callback function from member function
   boost::function<void (const pcl::PointCloud<pcl::PointXYZRGBA>::ConstPtr&)> f =
     boost::bind (&SimpleOpenNIProcessor::cloud_cb_, this, _1);

   // connect callback function for desired signal. In this case its a point cloud with color values
   boost::signals2::connection c = interface->registerCallback (f);

It will try to pass a boost::function to a std::function argument.

[taketwo]

One possibility for such situations is switch to using lambdas:

auto f = [this](const pcl::PointCloud<pcl::PointXYZRGBA>::ConstPtr& c){ this->cloud_cb_(c); };
boost::signals2::connection c = interface->registerCallback (f);

This will compile with both 1.10 and earlier versions, however requires that C++11 is enabled even with old versions.

[SergioRAgostinho]

One possibility for such situations is switch to using lambdas

You need to see from the consumer's perspective. Our users will still have in their codebases snippets like the one I mentioned above.

What about setting these function argument types to auto? Not really my first choice, but it won't break anything.

[taketwo]

You need to see from the consumer's perspective.

Sure, this is understood. Ideally the users should not need to change anything. But if that is not possible, then the goal is at least to make it possible to modify the code such that it compiles with both old and new PCL without horrible ifdefs.

What about setting these function argument types to auto?

Not sure I understand. You mean making callback type it a template parameter?

[SergioRAgostinho]

But if that is not possible, then the goal is at least to make it possible to modify the code such that it compiles with both old and new PCL without horrible ifdefs.

Now I understood the motifs behind your suggestion.

Not sure I understand. You mean making callback type it a template parameter?

Sorry. My brain froze there for a second and I messed up keywords and their respective uses. A second templated type on these methods would be an option. Can we leverage overload resolution to deprecate just boost::function though?

[taketwo]

To be specific, let's talk about this method:

pcl/io/include/pcl/io/grabber.h

Lines 233 to 234 in f9fb3f5

	template<typename T> boost::signals2::connection
	Grabber::registerCallback (const boost::function<T> & callback)

C++ supports template template syntax, we can rewrite the aforementioned method as:

template<typename T, template<typename> typename F> boost::signals2::connection 
Grabber::registerCallback (const F<T> & callback) 

I've never used this C++ feature, so can not predict what implications this may have on compiler's abilities to resolve template arguments. I've tried to build the library with this single change and compilation succeeded, this much I can tell :)

[taketwo]

By the way, my first proposal with lambda does not work, the compiler is not able to deduct template argument because lambda is not derived from const boost::function<Signature>.

[SergioRAgostinho]

C++ supports template template syntax, we can rewrite the aforementioned method as:

template<typename T, template<typename> typename F> boost::signals2::connection 
Grabber::registerCallback (const F<T> & callback) 

I suspect this won´t be enough. I feel that it's not capturing the variadic nature of function arguments. The typical function signature is defined like

template< class R, class... Args >
class function<R(Args...)>;

Nevertheless, try your proposed change on this PR just to see what happens.

[taketwo]

I feel that it's not capturing the variadic nature of function arguments.

Well maybe, but boost::function<T> is only capable of single argument anyway, so we are not losing anything there I think.

[SergioRAgostinho]

The changed types are either hidden behind typedefs or are compatible. In most cases the existing user code should still compile, right?

If the users made use of the typedefs it should be ok 😅 (famous last words)

I think we discussed this before. Preventing breakage would require some preprocessor conditionals to switch between boost/std and both agreed that those were ugly/terrible to keep around.

So we keep it like this I guess.

[taketwo]

I wonder if it makes more sense to not split this conversion over multiple PRs. Right now the approach we took seems reasonable, but what if we hit some limitations further down the road? I've grepped through the code base, we have a little short of 300 matches for boost::function, which is a lot, but still manageable for a single PR. What about adding per-module changes in separate commits and monitoring CI status?

[SergioRAgostinho]

It's fine by me. I have a copy of each module on each separate branch but can merge everything into this one. Do you want me push all the commits at once or one commit at a time so we can monitor the CI at each step?

[taketwo]

Oh, I assumed that you haven't done the conversion for other modules yet. Did you try to build the entire thing locally? If it compiles fine, then push everything at once, hopefully will work on CI as well.

[SergioRAgostinho]

Oh, I assumed that you haven't done the conversion for other modules yet. Did you try to build the entire thing locally?

I usually do it in a single go and then split every module into a separate branch. I only start these PRs once everything compiles on my environment.

If it compiles fine, then push everything at once, hopefully will work on CI as well.

Unfortunately I only build a subset of things due to the numerous 3rd parties :(

I'll address your review comment and proceed as discussed.

[SergioRAgostinho]

I just skimmed quickly through some of the new commits and I'm already noticing that some of these commits have non trivial changes. :/

[taketwo]

I'm still in the process of going through the commits. I noticed that now we have a lot of code like:

std::function<...> cloud_cb = boost::bind (&..., this, _1);

This compiles fine, but does not look nice. But I guess it's alright because we already have a pending PR that gets rid of binds altogether, right?

[SergioRAgostinho]

This compiles fine, but does not look nice. But I guess it's alright because we already have a pending PR that gets rid of binds altogether, right?

Exactly! I will pick those up after this batch of function replacements.

[taketwo]

Wasn't the idea to add FunctionT template parameter when we change boost to std in public API? Same way you did in the very first commit of this PR?

[SergioRAgostinho]

I forgot about it. I'll go over it tomorrow.

[taketwo]

Sounds right.

[SergioRAgostinho]

I just changed commits from d2986a3 (inclusive) onwards. Dropped 3 or 4 and added 2 at the end.

Edit: I'll create the deprecation PR for the grabber callback registration once this one is merged.

[taketwo]

1. I assume you want to squash some commits?
2. May conflict with Use using instead of typedef [common] #3118, which one should go first?

[SergioRAgostinho]

1. Yes
2. You can merge it. I'll fix things during the squash process.