Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 31, 2023
Merged

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

merged 1 commit into from
Oct 31, 2023

Conversation

non-det-alle
Copy link
Collaborator

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.

model/mac-command.cc Outdated Show resolved Hide resolved
model/mac-command.cc Outdated Show resolved Hide resolved
@non-det-alle
Copy link
Collaborator Author

non-det-alle commented Oct 26, 2023

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
Copy link
Member

pagmatt commented Oct 26, 2023

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 non-det-alle force-pushed the linting branch 2 times, most recently from 1969b44 to d52080d Compare October 26, 2023 13:27
@non-det-alle
Copy link
Collaborator Author

non-det-alle commented Oct 30, 2023

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).

@pagmatt pagmatt merged commit 926478b into develop Oct 31, 2023
0 of 2 checks passed
@non-det-alle non-det-alle deleted the linting branch October 31, 2023 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants