Conversation
boost/bind.hpp imports the placeholders at toplevel, which is a deprecated behaviour. Include boost/bind/bind.hpp instead and import the relevant namespaces when needed. Note that we should move to std::bind at some point
pierrewillenbrockdfki
left a comment
There was a problem hiding this comment.
Looks good to me. I am going to try this locally on our CI package-set and probably also a bit of runtime-testing.
|
Some small comments:
|
|
Found a compile issue, investigating. https://github.com/rock-data-processing/data_processing-orogen-type_to_vector Looks like nothing else broke, a very small check if our various runtime orchestrations tools still work came back positive. |
chhtz
left a comment
There was a problem hiding this comment.
Thanks a lot for cleaning this up!
My suggestions for improvement can also be done separately. I think there is also a lot of boiler-plate code which can be avoided now (assuming at least C++11 is used to compile)
| PropertyBase* PropertyBag::find(const std::string& name) const | ||
| { | ||
| const_iterator i( std::find_if(mproperties.begin(), mproperties.end(), std::bind2nd(FindProp(), name ) ) ); | ||
| const_iterator i( std::find_if(mproperties.begin(), mproperties.end(), std::bind(FindProp(), placeholders::_1, name ) ) ); |
There was a problem hiding this comment.
One can avoid the bind by passing name to FindProp, or just using a lambda:
std::find_if(mproperties.begin(), mproperties.end(),
[](const base::PropertyBase *b1 ){return b1->getName() == name;} );
| return 0; | ||
| } | ||
|
|
||
| base::PropertyBase* PropertyBag::getProperty(const std::string& name) const |
There was a problem hiding this comment.
This seems to be the same as PropertyBag::find? Maybe have one function call the other (and mark it deprecated)
| #include <boost/bind/bind.hpp> | ||
|
|
||
| using namespace boost::placeholders; |
There was a problem hiding this comment.
Maybe it would be a good opportunity to also replace boost::bind by std::bind?
| Property<T>* getPropertyType(const std::string& name) const | ||
| { | ||
| const_iterator i( std::find_if(mproperties.begin(), mproperties.end(), std::bind2nd(FindPropType<T>(), name ) ) ); | ||
| const_iterator i( std::find_if(mproperties.begin(), mproperties.end(), std::bind(FindPropType<T>(), std::placeholders::_1, name ) ) ); |
There was a problem hiding this comment.
Same as with getProperty, I would simply use a lambda here.
chhtz
left a comment
There was a problem hiding this comment.
I'm getting errors like the following at the moment:
In file included from /WS/tools/rtt/rtt/typekit/RealTimeTypekitOperators.cpp:44:
/WS/tools/rtt/rtt/typekit/../types/OperatorTypes.hpp: In instantiation of ‘class RTT::types::UnaryOperator<RTT::internal::identity<int> >’:
/WS/tools/rtt/rtt/typekit/RealTimeTypekitOperators.cpp:120:18: required from here
/WS/tools/rtt/rtt/typekit/../types/OperatorTypes.hpp:58:90: error: no type named ‘argument_type’ in ‘struct RTT::internal::identity<int>’
58 | typedef typename internal::remove_cr<typename function::argument_type>::type arg_t;
| ^~~~~
/WS/tools/rtt/rtt/typekit/../types/OperatorTypes.hpp:59:88: error: no type named ‘result_type’ in ‘struct RTT::internal::identity<int>’
59 | typedef typename internal::remove_cr<typename function::result_type>::type result_t;
| ^~~~~~~~
/WS/tools/rtt/rtt/typekit/../types/OperatorTypes.hpp:67:45: error: no type named ‘result_type’ in ‘struct RTT::internal::identity<int>’
67 | internal::DataSource<result_t>* build( const std::string& op, base::DataSourceBase* a )
| ^~~~~
This is likely caused by not inheriting from (the deprecated) std::unary_function anymore.
You are still building the typekit; only the default has changed to not build it, an existing build directory will keep building the typekit until you change the option. |
You are right, rtt builds after deleting the existing build-directory and running |
|
I changed the startegy ... Turns out that the void typeinfo is not provided by orogen (and I'd prefer adding support to this later). I now disable the problematic parts related to unary/binary functions if PLUGINS_ENABLE_SCRIPTING is OFF. This flag now disables the "operators" and "constructors" in the type info, which is what is actually depending on them. One gets essentially the same RTT if ENABLE_SCRIPTING is ON. @chhtz and @pierrewillenbrockdfki properly migrating to C++XX (we have to choose the target XX here) is not part of this PR. IMO we should first agree on a subset of RTT we want to support and move what we do not support (e.g. scripting for me) to a rtt-extras package (or remove it altogether ... it is after all our RTT) |
I'm fine with doing that at a later point. But if you change
I honestly don't know for sure which parts of RTT are needed where. I'm fine with separating optional parts into a different library or disable compiling them by default. |
|
@doudou Does it make sense to enable issues in this repository for discussing this, considering that it will not be merged back anytime soon? |
It would. From my perspective this is a hard fork. |
I don't. XX >= 11. But I really wanted to be as mechanistic as possible and not get into the whole "let's get rid of boost". Again, I'd be happy that we do it, but I have a 24.04 migration to finish first ;-) |
|
The compiler is allowed to assume all references are to valid objects; main difference to pointers(except this, which can also always be considred valid). The call sites that convert from a pointer need to do the checking, if any. |
This actually breaks backward compatibility (technically) since I did not update the internal typekit - which is not used within Rock
We do ignore it explicitly since the place where this happens is a crash handler ... that the write does not go through properly is the least of our worries
This fixes longstanding warnings about Eigen types on data ports, and possibly remove some UBs we are not aware of.
The compiler assumes it true, so it's probably elided at any optimization level anyways
|
@chhtz can you check that the problem is fixed now for you ? |
chhtz
left a comment
There was a problem hiding this comment.
LGTM
My previous comments can be addressed in a later change
I fixed most warnings related to the increase in C++ version (and boost as well). However, the PR disables the built-in typekit since it relies on type deduction that I did not want to resolve (and Rock should really not depend on that built-in typekit anyways)