Fix for issue 312: fixed clang-tidy linting warnings and error in include/* and src/*#310
Conversation
changed const to const ref in validator/uuid removed more linter warnings and errors fixed more linting errors linting fixes and pinned clang-tidy 13 in ci.yml fixed more liting errors fixed most warnings/errors except magic numbers and closure fixed linting errors in RpcClient.cpp fixed all errors, only warnings remainings test pipeline fixed all errors during build fixed signature mismatch in header file save builds and all tests pass replaced some magic numbers fixed magic numbers in uuid.cpp replaced all magic numbers modified PendingRequest in RpcClient.cpp with public constructor deleted unsued code removed unused setters
…k, need to keep UTransportMock.h
|
@PLeVasseur Would you be so kind to take a look at the PR and trigger the pipeline? Max is the colleague I told you about. An open question is whether the test files should also be linted. Personally, I prefer linting tests as well. But since many of the warnings and errors in the tests are of a different nature than in the source files, I created a separate issue for these, #311, which we could tackle next. |
|
|
||
| pushd "$PROJECT_ROOT" > /dev/null | ||
| for f in **/*.h **/*.cpp; do | ||
| for f in include/**/*.h src/**/*.cpp; do # test/coverage/**/*.cpp test/extra/**/*.cpp test/include/**/*.h; do |
There was a problem hiding this comment.
I believe there is no need to change the script in this way. If you want to limit the scope of the linter, you can provide the desired files explicitly.
.github/workflows/ci.yml
Outdated
There was a problem hiding this comment.
Thank you for pointing this out. This should now be fixed.
PLeVasseur
changed the title
PLeVasseur
changed the title
|
Code coverage report is ready! 📈
|
|
Hi @MaximilianToe -- thank you for the contribution! I noticed that there remained 10 or so warnings / errors in the |
Hi @PLeVasseur, Thank you for taking the time to review this PR. Sorry for the remaining errors, that was a mistake on my part. I forgot to run clang-format again before submitting the PR. I've now fixed the formatting and updated clang-format.sh to use clang-format 13. I reran the CI pipeline to see if everything is fine now. |
|
Code coverage report is ready! 📈
|
|
Hi @MaximilianToe, thanks for the contribution!
I confirmed that to be the case on the latest CI run. Great! I'll go review and approve. |
PLeVasseur
left a comment
PLeVasseur
left a comment
There was a problem hiding this comment.
Thanks again @MaximilianToe! I found one minor nit that I'll just go ahead and apply then merge.
|
Code coverage report is ready! 📈
|
3 participants
Fixed clang-tidy warnings and errors in all files contained in the include and src directories.
Also added the fixes suggested in Issue 264 and 270.
Additionally, updated and pinned clang-tidy version to 13 to make it consistent with the version of the clang compiler.
Lastly, modified the clang-tidy.sh script to only check include and src. Previously, this would check all files, including cmake files, etc.
The exact files to check should probably be discussed, and the script modified accordingly.
closes #312