[maintenance] cpp cleanup

[sgaist]

Motivation

I am opening this ticket as a followup to a discussion I am having with @incertum.

While working on #2890 and #2974, I saw potential to improve some things in the code base.

I am not currently talking about low level refactoring of core functionalities but improvement of the code itself to make the life of new and old falco developers easier.

What this ticket is not: a suggestion for a revolution.

Proposition

There are several aspects that I would like to help improve:

• code being more idiomatic C++ (for example, declare and use a variable in one line)
• const correctness
• override correctness
• avoid shadowing (variables, parameters or functions)
• ensure proper initialization

As a mean to that end, I have run cppcheck on the code base and it has found things in the above list and some more such as suggestions around using stl algorithms in place of loops in some places and the use of some deprecated APIs.

What I think would be nice is to devise a plan so that the various cleanup do not interfere too much with the current pull requests.

One possible way to go forward:

• Make PRs in engine and app separately even if the topics are the same
• Create PRs per type of cleanup (don't mix const correctness with override correctness)
• Decide if there are some things that should not change (for example keep loops as is rather than stl algorithms)
• Once done add a cppcheck action to the pipeline to ensure the code keeps its quality

Some questions around the organisation of that:

• Should there be one ticket per type of cleanup to track the work ?
• Use the task list feature of GitHub ?
• Should the cleanup happen in a separate branch that will be rebased regularly until all the cleanups are done ?

Further suggestions are welcome

Note: I know some changes might rather be style related when thinking about idiomatic C++ so another question would be should they wait for falcosecurity/libs#381 to be done ?

Tasks

Preview Give feedback

1. Fix override correctness #3063
   needs-kind +1
2. Fix explicit constructor #3068
   needs-kind +1
3. Fix const correctness #3082
   needs-kind +1
4. Improve initialization #3073
   needs-kind +1
5. Fix variable / function shadowing #3122
   needs-kind +1
6. Refactor variable initialization on use
7. Transform loop to std::algorithm
8. Improve exception throwing #3084
   needs-kind +1
9. Fix unread/unused variables #3096
   needs-kind +1

[incertum]

CC @falcosecurity/falco-maintainers - we will take a look soon! Thanks a bunch 🚀 !

[leogr]

Hey @sgaist

Thank you for your proposal! Here's my feedback.

• Create PRs per type of cleanup (don't mix const correctness with override correctness)

I guess this ☝️ would let us evaluate cleanups case by case and then 👇

Decide if there are some things that should not change (for example, keep loops as is rather than stl algorithms)

(sometimes, to decide if a change worth to be introducing or not, just a simple wip:/incomplete PR is enough)

Use the task list feature of GitHub ?

Yes, please 🙏

Should the cleanup happen in a separate branch that will be rebased regularly until all the cleanups are done ?

I believe the easiest way to move this forward is to make a PR per cleanup sequentially (each PR is a separate branch).

Note: I know some changes might rather be style related when thinking about idiomatic C++ so another question would be should they wait for falcosecurity/libs#381 to be done ?

This is the big question :)
That initiative has been blocked due to a lack of capacity. Before proceeding, we should at least agree on the overall style.

Let's see what other maintainers think about this.

[incertum]

we should at least agree on the overall style.

I would appreciate this as depending on the PR I also experienced varying style feedback 🙃 A more realistic timeline would perhaps be June/July 2024 (post Falco 0.38.0)?

[Andreagit97]

I believe the easiest way to move this forward is to make a PR per cleanup sequentially (each PR is a separate branch).

I agree to proceed in this way!

That initiative has been blocked due to a lack of capacity. Before proceeding, we should at least agree on the overall style.

Yeah at a certain point, we need to revamp this discussion!

[LucaGuerra]

Thank you for the suggestions! I am in favor of cleaning up the code, especially because lately we have been working to remove some potential sources of segfaults, undefined behavior and so on. I have some suggestions about it:

• You mentioned you used CppCheck to discover these issues and we also run CppCheck in our workflow but perhaps it needs a little maintenance so that it warns on the right things. Could you please edit our static analysis jobs to suggest some checks to add to our CI? If they are not enforced or at least mentioned your work will sadly be lost in a short amount of time.
• Out of all the cleanups, using algorithm is the one that sounds a bit more cumbersome because we have a lot of loops; personally I would keep that one out unless there is a very valid reason to do so, and that would require a bit more agreement
• We are doing something similar for runtime analysis as well (see Support sanitizers in Falco tests CI #2930 and cleanup(tests): consolidate Falco engine and rule loader tests #3066)

[sgaist]

• From the looks of it, the static-analysis.cmake contains a more aggressive configuration for cppcheck than the one I have been using. Currently my command is cppcheck -i ./build --enable=warning,style,performance,portability 2>err.txt. We might be using a different version of cppcheck though. I am running it from my macOS command line while building falco in a Linux docker container (It might be a bit of an exotic setup, I should rather spin a virtual machine to be working 100% in Linux).
  I am wondering whether there should be a reference platform for development like for example Debian 12, Ubuntu 22.04, etc. document. That way it might help new contributors be on the same page as the core developers.
• I agree with the algorithm part, it does not always make sense to move over to them just for the sake of doing it. A careful analysis is needed.
• Adding sanitizers is a very good idea. I am wondering whether doing some fuzzing would also be of interest.

[poiana]

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

[incertum]

/remove-lifecycle stale

[poiana]

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

[FedeDP]

/remove-lifecycle stale

[poiana]

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

[poiana]

Stale issues rot after 30d of inactivity.

Mark the issue as fresh with /remove-lifecycle rotten.

Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle rotten

[FedeDP]

/remove-lifecycle rotten