sierrafoxtrot / srecord

SRecord github Mirror
https://srecord.sourceforge.net/
GNU General Public License v3.0
42 stars 22 forks source link

Clang tidy fixes step 1 #32

Closed jtxa closed 1 year ago

jtxa commented 1 year ago

Here are some fixes based on clang-tidy warnings. Mostly auto-fixes, please see commit messages for details.

About half of the messages as discussed in #26 are fixed now:

bugprone-macro-parentheses
modernize-deprecated-headers
modernize-make-shared
modernize-raw-string-literal
modernize-redundant-void-arg
modernize-return-braced-init-list
modernize-use-auto
modernize-use-bool-literals
modernize-use-emplace
readability-container-size-empty
readability-delete-null-pointer
readability-duplicate-include
readability-isolate-declaration
readability-make-member-function-const
readability-non-const-parameter
readability-qualified-auto
readability-redundant-access-specifiers
readability-static-accessed-through-instance
readability-uppercase-literal-suffix

Additionally small manual fixes for:

bugprone-suspicious-string-compare
readability-const-return-type

Further changes were needed to get the CI running OK again. There were some updates to MegaLinter.

ATM the ubuntu mirror from MS Azure seems to be not reachable for the GitHub runner. So the tests on Linux may fail. Perhaps a temporary problem, I'll try to run them again later. If necessary I'll add an extra commit, that changes the mirror used.

jtxa commented 1 year ago

Created a new PR #33 for the CI fixes. That one should be merged first. Afterwards I'll rebase this PR and remove those commits from here.

jtxa commented 1 year ago

On MacOS these changes causes errors and warnings like:

error: no member named 'emplace_back' in 'std::list<std::string>'; did you mean '__emplace_back'?
warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]

I need to take a look how this has to be solved.

sierrafoxtrot commented 1 year ago

This is huge (but sensational Josef!). It's going to take a while to go through. Nights are mostly available, otherwise, I'll definitely be able to pour through on the upcoming weekend.

jtxa commented 1 year ago

I rebased it on top of #33 first (no code changes), and added the fix for MacOS: In CMake no C++11 was required until now, and on MacOS it seems the default was somewhere between C++98 and C++11. (The first 3 commit are included in #33, I hope they vanish here if you merge the other PR first. Or you merge this alone and cancel the other one, I don't know how GH works in this case.)

The review needs to be done per commit, that should be easier. I did compile each commit separately and compared the executable if they are binary identical to its predecessor. Hope this helps that you don't feel the need to review every single line of the mass auto-fixes in detail.

Commit Binaries
C++: Modernize and de-duplicate includes identical
C++: Use auto keyword identical
C++: Improve const usage differs
C++: Apply misc C++11 modernizations differs
C++: Remove void arguments for consistency identical
C++: Use uppercase literal suffix identical
C++: Apply misc readability fixes differs
C++: Fix macro parantheses identical
C++: Remove useless const identical
C++: Fix strcmp/memcmp identical
jtxa commented 1 year ago

Curious (but not concerned) to know why we are dropping abseil (the checks look handy). Guessing we violate something it checks?

Should have explained the current state of the rules. Today I use the default + those rules we currently check. Perhaps it's better to enable all, and then disable those we really don't want.

The initial .clang-tidy file just enabled those, that were running without problems. It was minimal to get the CI running. But because the analyzing time increased dramatically now, I have gone through all groups, and though about their usefulness. There are common rules, rules by companies or groups, rules for specific libraries.

This is what I guessed:

Enabled:

Currently not enabled:

Irrelevant: