signetlabdei / lorawan

An ns-3 module for simulation of LoRaWAN networks
GNU General Public License v2.0
182 stars 130 forks source link

Apply .clang-tidy linting rules to all files #146

Closed non-det-alle closed 10 months ago

non-det-alle commented 10 months ago

Apply ns-3 .clang-tidy linting rules to all files of the module.

Changes were applied automatically by running the combination of commands shown here with the additional -fix option of clang-tidy-diff.

This merge is meant to break up a previous huge pull request into more digestible pieces.

non-det-alle commented 10 months ago

I feel like this MR opened pandora's box... I'm very well aware that the code in this repo could benefit from many improvements (there are many more!): that's what I was saying since I proposed to integrate the version I maintained.

Let me know in this MR if there are linting-related things I can fix here, otherwise I propose to open dedicated future MRs (for instance, substitute range based loops in place of iterators). I'll commit each (type of) fix you point out and then squash them together when we are satisfied.

The main scope of the present MR was to fix linting warnings given by clang-tidy such that we could solve another failing test in the CI pipeline (#141) and proceed with its instantiation. Unfortunately, the next step in this direction would be to fix all 560 doxygens warnings of undocumented code we are currently getting 😭.

pagmatt commented 10 months ago

I feel like this MR opened pandora's box... I'm very well aware that the code in this repo could benefit from many improvements (there are many more!): that's what I was saying since I proposed to integrate the version I maintained.

Let me know in this MR if there are linting-related things I can fix here, otherwise I propose to open dedicated future MRs (for instance, substitute range based loops in place of iterators). I'll commit each (type of) fix you point out and then squash them together when we are satisfied.

The main scope of the present MR was to fix linting warnings given by clang-tidy such that we could solve another failing test in the CI pipeline (#141) and proceed with its instantiation. Unfortunately, the next step in this direction would be to fix all 560 doxygens warnings of undocumented code we are currently getting 😭.

I think we can keep in this MR the changes that are still somehow related to the linting, i.e., lines that have been changed by the code-linting tool and that, however, can be changed in a better way. I agree that the priority is to get this merged and finally merge the CI tests, as also for this it's a pain to check whether the changes make the test pass or not. In fact, I would merge this, and the CI tests right after, so that at least we can check some of the tests, even though some (like the doc ones) will inevitably fail for now

non-det-alle commented 10 months ago

Hello, I applied the changes indicated in you reviews + re-run clang-format (one of the clang-tidy changes was not respecting the formatting rules). I verified with this pipeline run on my fork that we are now passing the CI clang-tidy test. From what I can see, this is all for this MR.

I am still working on improving the overall CI pipeline system of #141 so do not approve that yet please. I will be submitting some changes there soon (to anticipate, I was able to implement docs deployment and code coverage, so I am planning to add them as manual jobs aside the per-commit pipeline).