Modernise mrpt-graphslam lib

[bergercookie]

Since we have started using C++11 all over MRPT after the 1.5 release we could refactor the mrpt-graphsalm lib accordingly. Following is a list of improvements that can be implemented to clean up and modernise that code:

• Use auto instead of explicit type declarations (especially in for-loops, assignment expressions) (see comments below)
• Replace 0 or NULL with nullptr
• Replace typedefs with alias declarations
• Prefer scoped enums (enum class) to C-style unscoped enums
• In C++11, prefer const_iterators to iterators
• Declare overriding functions override
• Declare functions noexcept if they won't emit exceptions
  noexcept functions are more optimizable than non-noexcept functions
• Use constexpr when possible instead of const
• Do not user-define a class destructor / class copy constructor if you aren't doing anything meaningful in them. That will deter C++ from generating the default move constructors. Replace empty constructors/destructors with =default
• Modify methods that take output arguments as pointers to take references instead.
  Even though such modification goes against the Google C++ Guidelines it's clearly the better approach since this way the function code doesn't have to check for null pointer and also the syntax for accessing that element is much clearer.
• Consider emplacement instead of insertion
  Using emplace/emplace_back/emplace_front creates the object directly inside the container (std::vector, std::map), avoiding the need to call the constructor and destructor of a temporary object.
• Use unique_ptrs / shared_ptrs instead of using bare new / delete - see CGraphSlamEngine_impl.h for examples on this. Allocate using make_shared<CLASS_NAME>(), make_unique<CLASS_NAME>() instead of shared_ptr<CLASS_NAME>(new CLASS_NAME(...))
• When possible use const_iterators instead of iterators cbegin, cend instead of begin, end
  Declare overriding methods as override in class inheritance (see e.g., the various policies deriving from CRegistrationDeciderOrOptimizer<GRAPH_T>)
• Replace for (iterator it=... loops with range-based C++11 for loops.

---

Link: https://github.com/bergercookie/mrpt/commits/mrpt-graphslam-improvements

[jlblancoc]

👍 to this initiative :-)

My only comments are regarding:

Use auto instead of explicit type declarations (especially in for-loops, assignment expressions)

Sometimes, leaving the full (lengthy!) types might help understanding the program. In those cases, I would consider leaving it. Now, all for loops should be ported to auto, or even better to the "for range" notation that avoids iterators at all.

Consider emplacement instead of insertion

Make sure of enabling all compiler warnings, or at least keep an eye on clang build logs (which seems to emit better warnings some times): in some cases, the "emplace" methods are not a gain at all, or even makes things worse if they disallow the use of RVO optimizations (copy elision).

Replace empty constructors/destructors with =default

Or even better: if no other ctor exists, just don't declare any ctor at all, given that all member variables are initialized to default values, e.g. via the int n{0}; notation.

Cheers!

[bergercookie]

On Sat, Mar 24, 2018 at 11:54 PM Jose Luis Blanco-Claraco < ***@***.***> wrote: 👍 to this initiative :-) My only comments are regarding: Use auto instead of explicit type declarations (especially in for-loops, assignment expressions) Sometimes, leaving the full (lengthy!) types might help understanding the program. In those cases, I would consider leaving it. Now, all for loops should be ported to auto, or even better to the "for range" notation that avoids iterators at all.

Yes, correct!

Consider emplacement instead of insertion Make sure of enabling all compiler warnings, or at least keep an eye on clang build logs (which seems to emit better warnings some times): in some cases, the "emplace" methods are not a gain at all, or even makes things worse if they disallow the use of RVO optimizations (copy elision).

Hmm, I wasn't aware of this. I am pro of the emplace_* functions instead of push_back alternatives mainly when using it as in the following case: // the first one is either more efficient or equivalent (in case of copy elision) than the second one as in the case of emplace_back the object is built in place // see also: https://stackoverflow.com/a/11876097/2843583 vec.emplace_back(x, y); vec.push_back(Point2d(x, y));

Replace empty constructors/destructors with =default Or even better: if no other ctor exists, just just declare any ctor, given that all member variables are initialized to default values, e.g. via the int n{0}; notation.

Only advantages of =default vs the implicitly defined constructor are that i) it's explicit and ii) it allows you to use C++11 attributes along it as described in this SO post: https://stackoverflow.com/questions/41219291/defaulted-constructor-vs-implicit-constructor?noredirect=1&lq=1 Cheers!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#697 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFjBj8A7yaEGtrgLxoSn3IMeYnOJICnCks5thtydgaJpZM4S5t_E> .

-- Nikos Koukis Robotics Engineer - SLAMcore [email protected] +44 902907732

[jolting]

This one suggests it is more efficient

mrpt/libs/graphs/include/mrpt/graphs/CDirectedGraph.h

Line 281 in 5336573

void getAdjacencyMatrix(MAP_NODEID_SET_NODEIDS& outAdjacency) const

This suggests it is less efficient.

mrpt/libs/graphs/include/mrpt/graphs/CDirectedGraph.h

Line 226 in 5336573

inline std::set<TNodeID> getAllNodes() const

With move semantics, the copy penalty is not a problem anymore.

The inline is unnecessary, but the 2nd way is preferred. The comments are now misleading.

Generally move semantics makes this efficient. Copy/move elision makes it efficient.

valueType getSomeValue() const
{i
  valueType val;
  val = ...some non trivial computation ...
  return val;
}

I think this form is prefered. I don't think it is necessary to have two versions of those getters.

[jolting]

Modify methods that take output arguments as pointers to take references instead.
Even though such modification goes against the Google C++ Guidelines it's clearly the better approach since this way the function code doesn't have to check for null pointer and also the syntax for accessing that element is much clearer.

If you have an output argument consider returning it as a value, unless there is some reason why the object needs to be mutated in place.

For multiple output arguments, C++17 will add structured-bindings. Until we upgrade to c++17 we can use std::tie.
https://skebanga.github.io/structured-bindings/

[bergercookie]

If you have an output argument consider returning it as a value, unless there is some reason why the object needs to be mutated in place.

Nice one... I hadn't really considered that seriously returning non-PODs by value!
Only caveat I see when copy elision is not possible (as in this example and if a user has already defined a custom constructor/destructor but not an explicit move constructor. Then C++11 will not call the default move constructor but the copy constructor instead (see the "Implicitly-declared move constructor" section: http://en.cppreference.com/w/cpp/language/move_constructor). In that case passinga reference to be modified is much faster as it doesn't involve any redundant copying.

[jlblancoc]

Sure! Returning by reference is still my preferred method. But it's good to know about the optimizations of returning by value for std::string, for example. Cheers!

[bergercookie]

Sure! Returning by reference is still my preferred method. But it's good to know about the optimizations of returning by value for std::string, for example. Cheers!

Oh, totally irrelevant, but do we have some sort of a C++17-like string_view functionality in MRPT?

The latter would be much more efficient than using std::string (could be used along with constexpr) and much more modern than using const char*

[jolting]

Is it already supported by clang 6, GCC 7.2 and vs2017?

[bergercookie]

I don't know...
It's certainly not C++11 though with which we should be compatible

[jolting]

We can use c++14 features.

[jolting]

clang-6 and gcc-7.2 both support string_view with the c++17 standard.
Considering we've already had to upgrade to 7.2 due to constexpr compiler bugs I think it is ok to turn on c++17 for string_view if @jlblancoc agrees and there aren't any issues with VS2017.

Since c++17 support isn't quite complete though any c++17 feature must be supported by all 3 target compilers.

I'd certainly prefer using std::string_view over implementing a custom one.

[jlblancoc]

@bergercookie Nope... we should (for now) stay up to C++14 for MRPT master (2.0).
The closest to what you said that we have already is string literals and such; see examples 2 & 3 here.

[jlblancoc]

Since c++17 support isn't quite complete though any c++17 feature must be supported by all 3 target compilers.

I don't remember the exact CMake commands to do this, but, it would be OK if we could set the general CMAKE_CXX_STANDARD to 14, then request additional individual features.

PS: I think it's target_compile_features(), but have to test it.
PS2: From a quick search, it seems std::string_view is only supported by MSVC 2017 starting at a particular "update" version. I have no big deal in bumping the required compiler any further for Windows if the current minimal versions for Linux are ok.

[jolting]

We could do what we did with llvm propagate_const until we want to bump the standard up to c++17.

[jlblancoc]

I just revisited this great page and found that C++17 is better supported than I imagined by GCC 7, CLANG 4 and even the latest MSVC 2017 releases.

It would be great to get rid of all those alignas() by using P0035R4's overaligned new operators; an incentive to bump towards C++17.

[jolting]

I just tested c++17. looks like issues in the boost python bindings.

[jolting]

CLevMarqGSO_impl.h needs an CConfigFile.h, CPlanarLaserScan.h include.

[bergercookie]

The closest to what you said that we have already is string literals and such; see examples 2 & 3 here.

Ok, yes, that's exactly what I wanted! 👍

[jolting]

Enabling C++17 may cause us to run into this bug.
boostorg/python#105

[jolting]

Looks like the issue is fixed in 1.65.0.
Might be a good reason to support Hunter.

[jlblancoc]

@bergercookie : I appended one more tasks to your list above, above converting loops to range-based loops, where applicable.

[jlblancoc]

On using string_view, it has some interesting numbers:
https://www.bfilipek.com/2018/07/string-view-perf.html

[jolting]

Worth watching around minute 20 to see why range based loops are better.
https://www.youtube.com/watch?v=bSkpMdDe4g4

[jlblancoc]

Awesome! I was really surprised of the closed-form summation replacement by clang (!)

[jolting]

Also if you ever wanted to know if you should return string, char * or string_view from a function, then here is your answer.
https://www.youtube.com/watch?v=9mWWNYRHAIQ

[jlblancoc]

Many of these "modernizations" can be greatly automated via clang-tidy: 37c5acb

I've started some of them, see the latest commits; so, some points above would be done not only for graph-slam, but for the entire mrpt ;-)