Port to C++14

[jlblancoc]

To be done (in 2017? -> yes) when it's safe to assume everyone has a decent C++14 compiler.

This implies:

• Deprecate support for Ubuntu precise (and also trusty but only in PPA, for the next item:)
• Use modern CMake 3.1+ C++ compiler features: target_compile_features, WriteCompilerDetectionHeader, set (CMAKE_CXX_STANDARD 11), etc.
• Smart pointers: forget about stlplus and move to C++11 smart pointers.
• Threads: move to std::thread, unless there are strong reasons not to do so.
• Switch tfest/indiv-compat-decls.h to <functional>
• CMake: use EXPORT, etc.
• Direct use of Eigen? Moved to its own issue: (For MRPT 2.0) Discussion on Eigen, matrix classes and alternatives for a faster build time #496
• Semaphore -> http://stackoverflow.com/a/27852868
• MRPT_OVERRIDE -> override
• CPipe : avoid deprecated auto_ptr<>
• Search and rewrite all areas having an #if MRPT_HAS_CXX11
• Drop mrpt::synch::CAtomicCounter in favor of C++11 std::atomic.
• Prefer nullptr
• MRPT_NO_THROWS ==> C++11.
• Make all destructors noexcept
• Consider replacing uint32_t enums with correct C++11 typed enums. E.g. in GNSS_BINARY_MSG_DEFINITION_START
• Review usages of mrpt::utils::delete_safe and consider changing them to use C++11 smart pointers.
• Foo::Create with make_shared ()
• std::function typedefs or similar for ZMQ wrappers: See ZMQ minimal support #231
• Support move semantics (read below)
• typedefs ==> using
• Add constexpr to geometry constructors, etc.
• alignas instead of Eigen align macros
• Simplify ctors via member initialization in headers, e.g. int a { 0 }; or int a = 0;
• Replace copy_ptr<>, etc. with simple using ... partial template specializations.
• Use using FP=... to define functors.
• Use template<> using ... instead of current aligned STL containers. ==> Nope: it would break existing code, it's not worth (Nov 3, 2017)
• Replace std::vector of fixed length with std::array. E.g. ReactiveNav classes, etc.
• Make sure all move ctors and move = operators are declared noexcept. Otherwise, they will not be eligible for "real move".
• refactor type traits
• Use weak_ptr for m_frame_tf API in ReactiveNav

Minimum compilers (see this table)

• MSVC 15 aka 14.1 (2017)
• GCC 4.8 (?)
• CLANG 3.3 (?)

Minimum CMake version: 3.1

[jolting]

"Important: Before GCC 5.1 support for C++11 was experimental. Some features were implemented based on early proposals, and no attempt was made to maintain backward compatibility when they were updated to match the final C++11 standard." - https://gcc.gnu.org/projects/cxx0x.html

I use c++11 features on Ubuntu 14.04(gcc 4.8.4) and it is usable.

It's worth noting that gcc 5.1 is the compiler used in Ubuntu 15.10. Ubuntu 16.04 LTS will probably use 5.3. Unfortunately, gcc 6 will probably not be ready before the 16.04 LTS, which will have gnu++14 as the default c++ dialect. I believe gcc 5.1 still defaults to gnu++98, so you'll have to add -std=gnu++11.

https://gcc.gnu.org/develop.html#timeline

[jlblancoc]

Thanks for the info!

I think that the limiting factor seems to be gcc in Ubuntu precise 12.04 (which we support in our PPA), which is supported until 2017/04 and comes with gcc 4.6.3.
Still, a usable subset of C++11 features was ready for gcc 4.6, so the porting may be possible this year without dropping support for precise.

[jolting]

CPipe uses std::auto_ptr which is deprecated in c++11. Not only is the warning annoying, but std::auto_ptr will be removed in c++17. Version 2.0.0 would probably be a good time to change that interface.

It might work to template that method. It should maintain compatibility to older code.

typedef std::shared_ptr<CPipeWriteEndPoint> CPipeWriteEndPointPtr;
typedef std::shared_ptr<CPipeReadEndPoint> CPipeReadEndPointPtr;

template <typename ReadPtr, typename WritePtr>
static void createPipe(ReadPtr &outReadPipe, WritePtr & outWritePipe)
{

[jlblancoc]

Thanks for the idea!
Sure, it'll be done for 2.0.0.

Also, you may have noticed that as a workaround to avoid those warnings I opened #235 , because CPipe is actually used in only a couple of places in the entire mrpt...

[jolting]

@jlblancoc probably want to add "support move semantics"

Sometimes you get move implicitly. Here are the rules.

Implicitly-declared move constructor
If no user-defined move constructors are provided for a class type (struct, class, or union), and all of the following is true:
there are no user-declared copy constructors;
there are no user-declared copy assignment operators;
there are no user-declared move assignment operators;
there are no user-declared destructors;

Eigen should support move already, but I don't think any mrpt classes have an explicit move constructors. It's very easy to break one of those above rules without realizing it. Probably the best place to start is to see if everything derived from CArrayNumeric supports move.

[jlblancoc]

Thanks! I added that to the list in the issue text above. Indeed, that should be done to allow users exploit move semantics.

If you have an idea of how to perform a generic compile-time or runtime test that fails if the "move" op. does not happen, we would add it to be done as part of the unit tests, just like it's done now with serialization ;-)

[jolting]

@jlblancoc

It's already in the standard.

This should give you good error messages:

#include <type_traits>

struct CannotMove
{
    CannotMove(const CannotMove &)
    {}
    CannotMove(CannotMove &&) = delete;
};

struct CannotCopy
{
    CannotCopy(CannotCopy &) = delete;
    CannotCopy(CannotCopy &&){};
};


template<typename T>
class ConstructorTests{
  static_assert(std::is_move_constructible<T>(), "Can't move");
  static_assert(std::is_copy_constructible<T>(), "Can't copy");
};

template class ConstructorTests<CannotMove>;
template class ConstructorTests<int>;
template class ConstructorTests<CannotCopy>;

gcc

example.cpp: In instantiation of 'class ConstructorTests<CannotMove>':
23 : required from here
19 : error: static assertion failed: Can't move
static_assert(std::is_move_constructible<T>(), "Can't move");
^
example.cpp: In instantiation of 'class ConstructorTests<CannotCopy>':
25 : required from here
20 : error: static assertion failed: Can't copy
static_assert(std::is_copy_constructible<T>(), "Can't copy");
^
Compilation failed

clang

19 : error: static_assert failed "Can't move"
static_assert(std::is_move_constructible<T>(), "Can't move");
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
23 : note: in instantiation of template class 'ConstructorTests<CannotMove>' requested here
template class ConstructorTests<CannotMove>;
^
20 : error: static_assert failed "Can't copy"
static_assert(std::is_copy_constructible<T>(), "Can't copy");
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
25 : note: in instantiation of template class 'ConstructorTests<CannotCopy>' requested here
template class ConstructorTests<CannotCopy>;
^
2 errors generated.
Compilation failed

Might want to add
is_move_assignable
is_copy_assignable

Also this might be useful for some specific tests.

template <class T, class U> struct is_assignable;

[jlblancoc]

Brilliant!

BTW: I just created a 2.0-devel branch to slowly start pushing changes there.

[jolting]

@jlblancoc It does look like there are some issues with move as shown by my pull request.
https://travis-ci.org/MRPT/mrpt/builds/134300599

/home/travis/build/MRPT/mrpt/samples/traits_test/test.cpp:36:3: error: 
      static_assert failed "Can't move"
  static_assert(std::is_move_constructible<T>(), "Can't move");
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/travis/build/MRPT/mrpt/samples/traits_test/test.cpp:43:16: note: in
      instantiation of template class 'TraitsTest<mrpt::poses::CPoint2DPDF>'
      requested here
template class TraitsTest<mrpt::poses::CPoint2DPDF>;
               ^
/home/travis/build/MRPT/mrpt/samples/traits_test/test.cpp:36:3: error: 
      static_assert failed "Can't move"
  static_assert(std::is_move_constructible<T>(), "Can't move");
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/travis/build/MRPT/mrpt/samples/traits_test/test.cpp:46:16: note: in
      instantiation of template class 'TraitsTest<mrpt::poses::CPointPDF>'
      requested here
template class TraitsTest<mrpt::poses::CPointPDF>;
               ^
/home/travis/build/MRPT/mrpt/samples/traits_test/test.cpp:36:3: error: 
      static_assert failed "Can't move"
  static_assert(std::is_move_constructible<T>(), "Can't move");
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/travis/build/MRPT/mrpt/samples/traits_test/test.cpp:54:16: note: in
      instantiation of template class 'TraitsTest<mrpt::poses::CPose3DPDF>'
      requested here
template class TraitsTest<mrpt::poses::CPose3DPDF>;

[jolting]

@jlblancoc: Might as well add c++17 to the list since they will be adding parallel policies. http://en.cppreference.com/w/cpp/algorithm

I think those would be preferred over Intel TBB based solutions.

[olzhas]

How can I join this initiative?

[jolting]

@olzhas Take a look at the 2.0 branch.
It's fairly stale right now. You'll want to merge master into 2.0. That would be a great first step.
Once you're done, fork it, send a pull request. Make sure it's to the right branch(your 2.0->mrpt 2.0).

I would really like to see stlplus shared_ptr replaced with std::shared_ptr.

Let me know if you need any help.

[jlblancoc]

By the way, this transition would be the perfect occasion to also add to each CObject, but only the ::Ptr start pointer as it is now, buy also a ::ConstPtr to const pointers, as in PCL, for example. This change would start in the macros in CObject.h

[jolting]

Agreed, I think supporting const will be a great second step. There's quite a lot of infrastructure with CObject which may make binding pointers smart pointers of different types a bit tricky, so I didn't want to overwhelm him. I think just the conversion to std::shared_ptr will be a good start.

I just didn't want to scare @olzhas away. Stripping out stdlib pointers is a considerable undertaking, so I wanted to limit the scope.

RE: CObject
I was thinking...there are some tricky things that happen with derived types, CObjects and pattern matching used for observations(i.e. IS_CLASS(obs, CObservation2DRangeScan)), which could be refactored into std::variant. It makes me wonder if the use of CObject adds undue complexity to each class. Perhaps delegating the responsibility that CObject currently has to several decoupled classes might make things simpler. Of course variants are c++17 feature(http://en.cppreference.com/w/cpp/utility/variant), so I was wondering if we should re-target the 2.0 milestone for c++17 prior to going too deep down the rabbit hole. Of course, removing CObject would certainly be a massive undertaking.

I think it's cleaner to have each class typedef their own smart pointer, but I think it would break a lot of what CObject currently does. For example:

class MyClass
{
      public:
        typedef std::shared_ptr<MyClass> Ptr;
        typedef std::shared_ptr<const MyClass> ConstPtr;
};

[jlblancoc]

Didn't know std:: variant, cool! Will see it's supported compilers,
though...

Just a note on your last proposal for typedefs: I'm on the phone now and
can't see it, but I'm almost sure that there is already a typedef named Ptr
inside each CObject, in one of the DEFINE_OBJECT* macros...
Only, that we also have the other macros outside of each class which are
redundant and define the FooPtr names, much more common since they were
used from the beginning of MRPT. But for simplicity, if this allows
dropping some of the required macros before and after each class
declaration, we could drop the FooPtr names and leave only Foo::Ptr.

We'll see...

[jolting]

I think variants are a late add to the c++17 standard. Work in gcc only started a little over a month ago.
It's similar to the boost variant though.
https://github.com/gcc-mirror/gcc/commits/master/libstdc%2B%2B-v3/include/std/variant

I have to admit that I still struggle with those DEFINE_OBJECT macros. I find them very confusing.

The nice thing about std::variants is that it that a single collection of variants can store very disparate types without requiring inheritance. It which helps preserve the sanity of your inheritance structure.

Also, the visitor pattern is almost like pattern matching from haskell.
http://en.cppreference.com/w/cpp/utility/variant/visit

[olzhas]

@jolting got it.

[jolting]

@olzhas This is what I had in mind with the variant:
jolting@0dbaa3c
For now the boost::variant will do, but I want to replace it with std::variant. I definitely don't have it all working yet, but I think it should make the macros unnecessary. It's not in any buildable state, so that's why it's on my work in progress branch.

I still need to tackle the type info thing and refactoring a couple of places where CLASS_ID is checked which could be simplified using templates(i.e. CPoseRandomSampler).

Take a look at CSerializable.h that uses the boost::variant for now. It should be trivial to replace this with std::variant.

Notable take a look at libs/base/include/mrpt/utils/CStream.h. It has a templated operator >> and operator << which should make conversion to and from CSerializableOption. boost::get should throw an exception if the types don't match, which can easily be caught and rethrown into a reasonable exception message.

Now each class just has a readStream, writeStream which can be called with a templated method for each observation type instead of using polymorphism, which seems like it's overkill for an interface.

[jlblancoc]

Hi @jolting !

wow, a lot of critical changes there :-)
One side that I like of your proposal is the drastic reduction of LOCs, of course.

But, on the other side, the std::variant idea is the only one in all "modern C++XX" for which I feel somewhat reticent. Let me motivate it:

• Scalability / maintanability: The list in jolting@0dbaa3c#diff-747ea90dd2801dd93b1c69954f1c4c0bR29 seems a weak point of the approach, doesn't it?? Right now, one can define new CSerializable objects in 3rd party libs (e.g. a private lib linked against mrpt) and the CSerializable mechanisms will work seamlessly with all the classes, inside and outside MRPT's source tree.
• How could one address the problem of deserialization without knowing the type in advance (e.g. a rawlog) with this? This comes down to the issue of having a class factory that, given a class name as a string, creates a new object of that type.
• The cost for Windows users: one feature I've struggled (that's the word!) to maintain over the years is keeping the dependency hell under control for Windows. If std::variant is not supported in MSVC2015... we enforce users to use Boost, which is a monster on itself... and which should be distributed in binary form for precompiled versions of MRPT.... doable, but until now I kept apart of it as much as I could ;-)

What do you think??

[jolting]

Dependency Hell

I think the variant type doesn't suit the problem exactly, but I think it's a step in the right direction. Perhaps the solution is to create our own type by leveraging some C++11 features.
I certainly don't want a final solution to depend on boost. It's just a means to an end, so I definitely want something that works easily on all platforms. Given that c++17 isn't available yet, I was thinking about creating an MRPT class that implements as much of the functionality with only c++11 features. Boost just had something that partially worked, so I thought I'd start from there.

Deserialization

In c++11 we can use type indexes and create a hash lookup for the type name.
Example: http://en.cppreference.com/w/cpp/types/type_index
This should provide the ability to go from type name to type fairly easily.

Scalability / maintanability

It seems that there is already partially a problem with this.
https://github.com/MRPT/mrpt/blob/c723c8fe02b96d3ae40dd5a8094762cd7ed0a588/libs/base/src/registerAllClasses.cpp

Actually what we really need is std::any(forgive me for throwing another c++17 feature in there)
http://en.cppreference.com/w/cpp/utility/any
I think we can probably implement something that does that partially in c++11 that will use the type_index and work for any type.

[jolting]

Forgive the prototype nature of the code, but Perhaps something like this:
jolting@653021a
Solves Scalability and Portability.

[jlblancoc]

Any is much, much better, thanks!! 👍

I may have lost something, but I think that the only missing critical feature of this approach (without the ugly old macros) is the lack of a class factory, required for reading from rawlogs, etc. right?

Do you think it's possible to do it somehow without relying on a central class registry, etc. etc. as it's done right now?

[jolting]

I think so, but I'll have to mull it over before trying some more brutal refactoring tricks.

I believe the context in which the rawlog is read really decides what types observations really need to be in the registry. You shouldn't need code to de-serialize an object you never intent to use(I think). Maybe the registry can just be local for the types that you want to read.

RawLogReader<OBS1,OBS2,...OBSn> reader;

[jlblancoc]

Just to keep this thread alive: during these months I've been updating the checklist at the top with related tasks. I hope we soon will have time to finally release 1.5.0 and put hands on 2.0.0!

The only change I'm still reluctant to undertake is a replacement based on Any above... It's more elegant, agreed, but I can't see how to maintain the flexibility of registering classes from different DLL's/.so's dynamically, etc. The current system is a bit ugly with all those macros, but is really well tested... and works! :-) We'll have to think much more on this particular item.

[jolting]

I don't think it would be useful to check that every class to be movable universally. The PDF variants of the poses definitely have the most to gain from move semantics and I think that is already covered. That particular item can be checked off the list.

[jlblancoc]

FYI: Today I finally removed the branch mrpt-2.0-devel. I verified that a previous GH fork with changes in that branch, can open a Pull Request against master and git catches up. Cheers!

[jlblancoc]

Update: 98aaaed removes our home-made make_vector<> since the C++11 braced initialization is far better and more general.

[jlblancoc]

Point "CMake export" moved to a new issue #602 . It actually has nothing to do with C++ versions.

[jlblancoc]

Done with "Add constexpr to geometry constructors, etc."

[jlblancoc]

Done (mostly) with

Simplify ctors via member initialization in headers, e.g. int a { 0 }; or int a = 0;

[jlblancoc]

Just deleted this one from the list, since it didn't have actually anything to do with C++14.

• Create Qt-like macros to easily declare simple member variables + getter + setter, eg. DECLARE_PROPERTY(double, Width, m_width ) --> double m_width; double getWidth() const;, ...

Also, I'm unsure about it usefulness right now...

[jolting]

Use weak_ptr for m_frame_tf API in ReactiveNav

I think the weak_ptr/shared_ptr muddies the interface a little all to avoids the somewhat trivial copy.

You could pass it in as a parameter as well. That should avoid the copy as well.
What do you think of passing it as an optional param like nav.navigationStep(frameTF)? I guess the biggest problem that is you'd have to change a few signatures.

navigationStep(frameTF)->performNavigationStepNavigating(true, frameTF)->performNavigationStep(frameTF)
navigationStep(frameTF)->waypoints_navigationStep(frameTF)->performNavigationStepNavigating(false,frameTF)->performNavigationStep(frameTF)

[jlblancoc]

I think the weak_ptr/shared_ptr muddies the interface a little all to avoids the somewhat trivial copy.

I also had doubts about whether it's worth. Anyway, there should be no need for signature changes, unless I misunderstood you, since we already have void setFrameTF (mrpt::poses::FrameTransformer< 2 > *frame_tf). That pointer is the one that I was not 100% comfortable with. I think it might be only edge cases where the weak_ptr are required, since in a normal app lifecycle I don't imagine a situation when the user destroys the TF object while the RNAV engine is still alive.
Worth a thought...

[jlblancoc]

On this one (almost the last one that remains in this ticket!):

refactor type traits

I remember is what a @bergercookie 's thing related to traits of poses, but I don't know if that code is still around and where it is, etc. Any update on that part? I'll be happy of refactoring it, but just wanted to be sure it's still needed ;-)

[jlblancoc]

Make sure all move ctors and move = operators are declared noexcept. Otherwise, they will not be eligible for "real move".

I'm marking this one as done after reviewing that the condition is fulfilled for "lightweight" and "regular" point and poses, and for matrices.

[jolting]

Which type traits?

[jlblancoc]

Which type traits?

That's the point... perhaps I'm making it up, but I could swear Nikos added some trait classes to the old mrpt-base module and we all agreed they could be refactored someday, but I just can't locate them now :-)

[jlblancoc]

Use weak_ptr for m_frame_tf API in ReactiveNav
done.

On the mysterious missing "traits", I'll just mark it as done and if @bergercookie thinks it's something still in the to-do list, it could be added to #697.

So, after almost 4 years, I think we can consider this task list as done... yay!! 🎉

[jolting]

https://github.com/MRPT/mrpt/blob/80c44e1114fb113b31b56577eec7b80fb48402e6/libs/graphslam/include/mrpt/graphslam/types.h

[jlblancoc]

https://github.com/MRPT/mrpt/blob/80c44e1114fb113b31b56577eec7b80fb48402e6/libs/graphslam/include/mrpt/graphslam/types.h

Thanks, that one was on my radar, but it is fine as is: it defines graphslam-specific traits. The one I recall was related to poses and my thoughts then were that it seemed easily merge-able with mrpt::poses::SE_traits<> (?).