yarp icon indicating copy to clipboard operation
yarp copied to clipboard

PortReport deprecated in favor of modern callbacks

Open aerydna opened this issue 6 years ago • 11 comments
trafficstars

the pr makes possible to pass a callable to everywhere the PortReport was used.

example port.setReportCallback([ ](const yarp::os::PortInfo & info) { //do things }); the major difference when passing a functor that has member variables that changes inside the operator() is that its required to wrapping it with std::ref()

aerydna avatar Jun 06 '19 16:06 aerydna

I don't agree, having to declare a class just to add a callback is confusing and very suboptimal.

drdanz avatar Mar 04 '20 17:03 drdanz

Just to understand, it would be helpful to provide an example of how an existing piece of code that uses a class that inherits from PortReport will change with this new pattern.

traversaro avatar Mar 04 '20 17:03 traversaro

@drdanz my point is that the class might be responsible for the callback plus some other handling; it very much depends on what type of computation is required.

Examples of this new usage that cover a wide range of cases may enrich the PR.

Marking with 👎 anyone who doesn't agree with you is not a common attitude in our community @drdanz

pattacini avatar Mar 04 '20 18:03 pattacini

Marking with :-1: anyone who doesn't agree with you is not a common attitude in our community

That was not personal, you proposed Why don't we add up this new functionality on top without deprecating the good old method? and I expressed my opinion against that proposal.

drdanz avatar Mar 04 '20 18:03 drdanz

my point is that the class might be responsible for the callback plus some other handling; it very much depends on what type of computation is required.

This can still be done by passing a member of the class as callback

drdanz avatar Mar 04 '20 18:03 drdanz

my point is that the class might be responsible for the callback plus some other handling; it very much depends on what type of computation is required.

This can still be done by passing a member of the class as callback

Just to understand, this means that:

PortReportChild callbackFunctor;
port.setReportCallback(callbackFunctor);

becomes:

PortReportChild callbackFunctor;
port.setReportCallback(std::bind(&PortReportChild::report, &callbackFunctor, _1));

?

traversaro avatar Mar 04 '20 20:03 traversaro

@drdanz, of course, that wasn't personal, I was referring to a different context and attitude.

Anyway, my major concern was about the in-place definition of the callback as it was remarked from the example given in the PR description.

Lack of clarity in the PR description generates confusion and misunderstanding that we can easily avoid by providing examples – or at least more details – as already mentioned. This is because we don't have always time to review/peruse the PR content.

pattacini avatar Mar 04 '20 21:03 pattacini

Either std::bind or (better) using a lambda:

port.setReportCallback([](const yarp::os::PortInfo& info){ PortReportChild::report.callbackFunctor(info); });

drdanz avatar Mar 04 '20 21:03 drdanz

Either std::bind or (better) using a lambda:

port.setReportCallback([](const yarp::os::PortInfo& info){ PortReportChild::report.callbackFunctor(info); });

I guess if you have an instance of PortReportChild , you should do as:

port.setReportCallback([&callbackFunctor](const yarp::os::PortInfo& info){ callbackFunctor.report.callbackFunctor(info); });

traversaro avatar Mar 04 '20 21:03 traversaro

@drdanz, of course, that wasn't personal, I was referring to a different context and attitude.

Again, I have no idea what you are talking about. :-1: is just a reaction to a comment. If we have some misunderstanding, feel free to come talk to me.

I agree that some example might be useful, but nonetheless, this is a very basic C++11 callback approach that has been around for about 9 years. It's more or less the same that is used in almost any std algorithm, I don't think it requires a big explanation.

drdanz avatar Mar 04 '20 22:03 drdanz

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

:white_check_mark: drdanz
:x: aerydna
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Apr 20 '20 15:04 CLAassistant

closing for obsolescence

aerydna avatar Jan 02 '23 13:01 aerydna