Modernize ARGoS build flags

[jharwell]

Hi @ilpincy,

I'm wondering if you would be open to updating the GCC/clang warning flags used to build ARGoS, adding additional flags depending on the GCC/clang version that is available? Or also updating the minimum required version of clang/GCC so more warnings are available?

I did a quick test with GCC9 with the following additional flags which I generally use in my own C++ code:

-W -Wall -Wextra -Wcast-align -Wcast-qual -Wdisabled-optimization -Wformat=2 -Winit-self -Wlogical-op -Wmissing-declarations -Wstrict-overflow=5 -Wsuggest-attribute=format -Wsuggest-final-types -Wfloat-equal -Wshadow -Wredundant-decls -Wstrict-overflow -Wswitch-default -Wundef -ansi -Wpointer-arith -Wno-unknown-pragmas -Wstack-protector -Wunreachable-code -Wmissing-format-attribute -Wfloat-conversion -Wnarrowing -Wmultistatement-macros -Weffc++ -Wunused-macros -Wstrict-null-sentinel -Wclass-memaccess -Wsign-promo -Wnoexcept -Wold-style-cast -Woverloaded-virtual -Wctor-dtor-privacy -Wdelete-non-virtual-dtor

This is resulted in a LOT of additional warnings, some of which might be overkill for ARGoS, but there are some which are worth paying attention to and fixing, such as those for comparing floating point values via == or != , or when member variables are not given a default initialization when they are declared or initialized in the member initialization list, or when there is an implicit float -> int cast.

If this is something you would be open to, I would be happy to contribute to modernizing ARGoS in this way :-).

[allsey87]

To throw in my two cents, I think modernizing a code base (by fixing issues raised by these flags) from time to time is a good idea, however, only if a really extensive set of automated tests exist. Without these tests, I feel like such a modernization effort would be in a net negative due to the unavoidable and hard to find bugs it could create

[jharwell]

Would a good rule of thumb then be that in order to fix any warnings generated by a specific class, that a comprehensive suite of unit tests for all public member functions exist? For some core classes (e.g. CVector2, CVector3), that would not be very hard, as they are easy to test in a non-integrated context.

[ilpincy]

Modernizing the ARGoS code is becoming increasingly important. I was skeptical of using modern C++ because of the patchy support it had on embedded compilers, but now it's really overdue.

There are many things that should be changed. Threading support really needs std::thread; the double dispatch trick I made would probably work better if written with std::function; and many optimizations that were necessary in C++03 because of the lack of move semantics are now just clunky.

The issue is that this is quite a lot of work, and many interfaces will break (not considering bugs).

[jharwell]

I was thinking more in terms of purely fixing compiler warnings enabled by additional flags, but I agree that switching to STL constructs (where possible) in the code would also be beneficial. std::function, std::thread, std::shared_ptr, std::unique_ptr all come to mind.

It would be good to start with just fixing compiler warnings, as those can be fixed more easily, and much less prone to programmer error, I think. Perhaps starting with the robot plugins under plugins? That is the least integrated area of ARGoS, and changes could be performed in a fairly isolated way. It would probably be a good idea to create a separate branch off of master which all modernizing changes could be committed to, to minimize potential issues for users.

[ilpincy]

Yeah, starting from the plugins is a good idea. The e-puck is a good starting point because not many people use it. And yes, a dedicated branch would be good.

[jharwell]

Looking at the epuck entity, I see that all of the *EquippedEntity components are adding to the robot via new and AddComponent(), but I don't see where they are deleted. I've grepped through the ARGoS codebase for delete and can't find where this is happening--can you point me in the right direction ?

[ilpincy]

It's done in these two places:

https://github.com/ilpincy/argos3/blob/master/src/core/simulator/entity/composable_entity.h#L206

https://github.com/ilpincy/argos3/blob/master/src/core/simulator/space/space.h#L317

[ilpincy]

On a different topic, but related, I am working on changing the logic that creates components. The change would make it so components are created only when necessary, saving tons of memory when lots of robots are used.

[jharwell]

Ah I see--thanks! So one change I think would be good to make (starting with the e-puck) is to have the root robot entity own all of its components via std::unique_ptr, so that when it is destroyed all of its components will automatically be cleaned up as well. This would require 2 changes, as far as I can tell:

• AddComponent() overload which takes a raw pointer rather than a reference. indicating non-ownership of the passed object.
• Adding a STLManaged() attribute to all entities (defaults to false) indicating if the entiy is managed by the STL via std::unique_ptr or managed by ARGoS via a raw pointer. This would allow the CSpace::RemoveEntity() function to call delete on root entities only, and to also allow incremental switching to std::unique_ptr, with the current behavior being exactly maintained during that process.

What do you think?

[ilpincy]

What would be the advantage of this approach as opposed to the current code?

[jharwell]

Using smart pointers generally makes code much less prone to memory related bugs, because reference counting, freeing the memory once you are done with it, etc, all happen automatically. std::unique_ptr especially was designed to be as fast as raw pointers (as long as you don't use a non-trivial deleter with it); std::shared_ptr does have some memory overhead due to locking and reference counting.

It also makes the code clearer as to who owns what by documenting ownership: if an object has a std::unique_ptr as a member, then it owns the pointed-to object, and if it has a raw pointer, it doesn't, so classes that get access to the managed object via a raw pointer know they don't need to worry about cleaning up memory that it points to, because they don't own it. If I look at the CEPuckEntity, I couldn't tell at a glance if its pointer members were owning, or if they passed the ownership to CComposableEntity via AddComponent() (which they effectively did, as the CEPuckEntity destructor does not clean them up when it was destructed).

Smart pointers also force you to be explicit with transfers of ownership when copying an object via std::move which makes you think twice about if you really need to copy an object or just reference it somehow.

[jharwell]

An alternative change would be to make the entity indexes maintained by the space owning via std::unique_ptr, and have all the entities have non-owning references to them, but I think that is less intuitive.

[ilpincy]

My question was more why we should mix std::unique_ptr and classical pointers, adding an extra flag to distinguish the two cases. I have a bit of experience with C++, I know how smart pointers work :-)

[jharwell]

Ohhhhh I see--my bad.

I guess this line of reasoning is more about the scope of modernizing ARGoS that you want to do, in terms of how intrusive to be. Changing floating point comparisons, member variable initialization, etc. are all changes that are localized to a class (and stem from fixing compiler warnings), but for some of the higher level changes (like memory management and refactoring move semantics) it is much more difficult to change them in just one part of ARGoS.

Thinking about it more, ARGoS is large enough that switching its memory model in one shot, even if you don't introduce any bugs in the process, is still a daunting task from a coding standpoint. Breaking it up into smaller pieces and changes one part of ARGoS at a time would be somewhat problematic, as mixing memory models will make the code harder to understand than having everything in one model.

So I guess my question is what is the list of things you'd like to do for modernizing (at least initially), and what are things to leave on the shelf for now? Is it better to go "breadth first", fixing all compiler warnings everywhere, then going into higher level things like memory management, or to go "depth first", and apply all modernizing changes to each part of ARGoS in sequence (starting with the e-puck, as you suggest) ? As for which is better, I really don't know. I've never tried to do something like this on a project the size of ARGoS before.

[ilpincy]

As for compiler warnings, I would first pick those in -Wall for both clang and gcc. I work on the most recent Mac OSX, so at least for clang, compilation warnings are under control. For the latest gcc I haven't done much, but it's simple to handle. As for -Wextra, my policy has been to be lazy. Those warnings do not necessarily translate into bugs, but it's good to fix as many as possible.

As for using modern features, I think that I would proceed in this order:

1. It makes sense to replace pthread with std::thread immediately.
2. It also makes sense to rewrite most for loops with modern syntax and update the variable declarations using auto when it makes sense.
3. I am still on the fence regarding random number generation, but this would be an easy next step. I imported verbatim the original source of Mersenne Twister from gsl (which in turn is identical to the code on the Mersenne Twister webpage). I need to check whether using std::mersenne_twister_engine would change things or not, but there's an obvious advantage in using that.
4. There is value also in using std::function instead of my hand-made function pointers, for readability and type safety.
5. I think that the code (especially in the math library, but also in general) should be changed to exploit tail recursion and move semantics. This is quite a big change and it will break people's code, but it's worthwhile.
6. The last thing I'd do is using smart pointers. This is possibly the trickiest change of all, because it impacts the ARGoS core very deeply. There is value in going in that direction, so it should happen at some point.

[jharwell]

That list sounds like a good plan! I'll start with the compiler warnings and #2 in the robot plugins--I should have some time to start on that this week :-). My std::thread knowledge is a bit rusty, but I should be able to do that without too much issue as well.

[ilpincy]

If you fix warnings and for loops, I can look into std::thread myself.

[jharwell]

I started looking at fixing floating point comparisons, and ran into the following issue: Bullet and Chipmunk both use the same compile flags as ARGoS, so turning on -Wfloat-equal gives you tons of warnings in those libraries. This can be fixed when compiling ARGoS files which include those headers by specifying cmake SYSTEM for the Bullet/Chipmunk include directories, and changing include paths accordingly. But that doesn't fix the issue when compiling the source for these libraries.

My instinct is that modifying the physics libraries that ARGoS uses at all is not a good idea, so I'm trying to figure out a way to work around this, and here is what I've come up with as possibilities, ordered from least to most invasive:

• Don't worry about floating point equality comparisons in ARGoS/Bullet/Chipmunk, as they don't seem to be affecting things negatively from a numerical stability standpoint. Unpredictable behavior can still result insome cases, depending if the values to be compared can be exactly represented in base 2, but ARGoS might not be sensitive enough to them to warrant fixing the warnings introduced by adding -Wfloat-equal.

• Create two sets of build flags: one for ARGoS and one for ARGoS external libraries (e.g. Bullet, Chipmunk, TinyXML), with the only difference being the warning options that are enabled (and therefore should be fixed if they are triggered). Manually modify the necessary sub-CMakeLists.txt files to choose the correct set fo compile warnings for a given directory under src/.

• Move Bullet, Chipmunk, TinyXML to their own prefix under src/ext(or something similar), and use the existing set of compiler warnings for those subdirectories, and add additional warnings (like -Wfloat-equal) to the set for files in ARGoS proper. This feels more like the "correct" way to do it from a software engineering pov, but is also much more invasive of a change.

What are your thoughts?

[ilpincy]

I would ignore -Wfloat-equal altogether. When I coded the sensors I was fully aware of the issues of floating-point comparisons.

[jharwell]

Excellent. Similar question: For many of the OpenGL calls (e.g glVertex3f()), warnings are thrown with -Wfloat-conversion when ARGOS_USE_DOUBLE is true. Is there value in changing them all to glVertex3d(), because ARGOS is usually built with Real=double when building for the simulator and compiling OpenGL ? There are other implicit floating point -> int implicit conversions which probably should be casts if they are intended to truncate elsewhere in the code, but this warning flags both kinds of conversions.

[ilpincy]

Yeah, replacing glVertex3f() with glVertex3d() makes sense. I have a draft for a new visualization that uses the modern OpenGL 3 interface, but never had the time to finish it...

About double to int conversions, where are the warnings?

[jharwell]

They are in colored_blob_perspective_camera.cpp, rng.cpp, rate.cpp, simulator.cpp, range.h, floor_entity.cpp, led_medium.cpp, and radio_medium.cpp

[ilpincy]

OK, so pretty much everywhere. Can they all be solved with static_cast<UInt32>(...)? I guess we need to go case-by-case.

[jharwell]

Most of them can, but there are a few I'm not sure of. I will go ahead and static_cast<UInt32>(...) for all, and mention the ones I'm really not sure of in the pull request.