[WIP] Clang tidy fixes

[jtxa]

ATTENTION: this PR is not for a detailed review.

Please have a rough look at each commit (maybe just the first 2-3 changes), which kind of auto-fix makes sense. I will post my conclusion hopefully within a day (I do not have time now).

When we decide on which fixes shall be applied, I will look into every change in detail.

Short summary up-front:

• all look useful
• modernize-loop-convert maybe needs loop variables renamed for clarity
• modernize-use-trailing-return-type is weird, I would not apply it

[jtxa]

We should definitely not apply fixes that break the build ;-) But maybe that was caused by manually rebasing it. For this first review the rebase was faster then re-applying with the tool. For the real change I will run the tool again (I have that automated). And check if it builds.

[jtxa]

• cppcoreguidelines-pro-type-member-init: ok
• cppcoreguidelines-init-variables: ok; manual improvement: some are unnecessary when we do not declare variables at block begin as in C
• bugprone-macro-parentheses: must fix
• bugprone-implicit-widening-of-multiplication-result: must fix; should be reviewed, if multiplication really fits into same type, maybe add assert; clang-tidy in ubuntu 2204 seems buggy, the bracket was inserted at wrong position
• readability-uppercase-literal-suffix: fix; it's a must for L, but other static code analyzers (and coding standards) I know, they do uppercase for every suffix; so it's probably just u->U changes
• readability-static-accessed-through-instance: ok; see readability-convert-member-functions-to-static below
• readability-redundant-string-init: fix
• readability-redundant-member-init: fix
• readability-redundant-access-specifiers: fix
• readability-qualified-auto: fix
• readability-non-const-parameter: fix, const helps preventing bugs
• readability-make-member-function-const: fix, const helps preventing bugs
• readability-isolate-declaration: must fix; these cases are simple, but it helps preventing bugs
• readability-inconsistent-declaration-parameter-name: must fix; replaced in decl, so must be reviewed if the other name is better; helps avoid confusions when reading code
• readability-implicit-bool-conversion: ok
• readability-else-after-return: fix, needs manual review, maybe code can be improved
• readability-duplicate-include: must fix
• readability-delete-null-pointer: fix
• readability-convert-member-functions-to-static: ok; needs manual review (arglex ok, ppb ok or move as local helper into .cc, os65v is maybe unused)
• readability-container-size-empty: fix
• readability-braces-around-statements: ok; this is not the current style, but makes conditional things more obvious (usually required by safety code standards)
• modernize-use-using: ok
• modernize-use-trailing-return-type: no; looks weird, not mentioned in C++ Core Guidelines
• modernize-use-override: must fix
• modernize-use-nullptr: must fix
• modernize-use-equals-delete: fix
• modernize-use-equals-default: fix
• modernize-use-emplace: fix
• modernize-use-default-member-init: fix
• modernize-use-bool-literals: must fix
• modernize-use-auto: fix
• modernize-return-braced-init-list: fix
• modernize-redundant-void-arg: ok; this is not the current style (most of the times), but recommended by C++ Core Guidelines
• modernize-raw-string-literal: fix
• modernize-pass-by-value: ok
• modernize-make-shared: ok
• modernize-loop-convert: must fix; maybe needs loop variables renamed for clarity
• modernize-deprecated-headers: must fix

My wording:

• must fix: I guess everyone will agree (with the goal to avoid bugs)
• fix: useful, most people will agree
• ok: useful (but I don't have an opinion, I'm usually stuck to C++98 myself)
• no: I wouldn't do it

[sierrafoxtrot]

Ok, I didn't realise you were still working on this at the same time as me. I'm trying desperately to avoid doing that, sorry. I'll go back through my list of comments and review your assessment. Please, hold off updating for a few hours to give me a chance to get through it.

[sierrafoxtrot]

I hope this summary list below still lines up with the review comments above. If in doubt, the review comments were written later if that helps).

Summary of responses. The review comments might site specifics etc but the main comments are here: - SF

• cppcoreguidelines-pro-type-member-init: ok

SF: discuss (or default to no): As mentioned in my review comment, clang-tidy didn't seem consistent in when it triggered on this. In principle, the check itself is ok.

• cppcoreguidelines-init-variables: ok; manual improvement: some are unnecessary when we do not declare variables at block begin as in C

SF: yes (and agree with your approach to the fix)

• bugprone-macro-parentheses: must fix

SF: yes

• bugprone-implicit-widening-of-multiplication-result: must fix; should be reviewed, if multiplication really fits into same type, maybe add assert; clang-tidy in ubuntu 2204 seems buggy, the bracket was inserted at wrong position

SF: Yes. Your notes make more sense than the changes clang-tidy provided LOL. Definitely keen for this to be a well written and reviewed fix!

• readability-uppercase-literal-suffix: fix; it's a must for L, but other static code analyzers (and coding standards) I know, they do uppercase for every suffix; so it's probably just u-U changes

SF: YES!

• readability-static-accessed-through-instance: ok; see readability-convert-member-functions-to-static below

SF: yes: If these are a consequence of other changes making things static that weren't previously, then lets go with it.

• readability-redundant-string-init: fix

SF: yes

• readability-redundant-member-init: fix

SF: yes (my review comments danced around on this one but seems more in keeping with moving defaults to declarations.

• readability-redundant-access-specifiers: fix

SF: yes

• readability-qualified-auto: fix

SF: yes (as auto is now being used)

• readability-non-const-parameter: fix, const helps preventing bugs

SF: yes

• readability-make-member-function-const: fix, const helps preventing bugs

SF: yes

• readability-isolate-declaration: must fix; these cases are simple, but it helps preventing bugs

SF: yes

• readability-inconsistent-declaration-parameter-name: must fix; replaced in decl, so must be reviewed if the other name is better; helps avoid confusions when reading code

SF: yes (but agree the more meaningful name isn't always the one that has been suggested here. In most cases, the header is better which makes sense, peter always wrote interfaces first almost to completeness and then filled in the implementation.

• readability-implicit-bool-conversion: ok

SF: no, (or perhaps discuss). In some instances the implicity bool makes sense isupper() reads nicely to humans. But boolean checks against zero make me queasy. On balance, no but happy to chat more.

• readability-else-after-return: fix, needs manual review, maybe code can be improved

SF: discuss: I like the idea of code review/improve. But I do think the current form is easier to read in terms of intent.

• readability-duplicate-include: must fix

SF: yes

• readability-delete-null-pointer: fix

SF: yes (a holdover from earlier times)

• readability-convert-member-functions-to-static: ok; needs manual review (arglex ok, ppb ok or move as local helper into .cc, os65v is maybe unused)

SF: yes but agree on the review.

• readability-container-size-empty: fix

SF: yes

• readability-braces-around-statements: ok; this is not the current style, but makes conditional things more obvious (usually required by safety code standards)

SF: yes. (My current day-job is on medical devices. Can confirm!)

• modernize-use-using: ok

SF: no (my default is to leave it unless there is a benefit.)

• modernize-use-trailing-return-type: no; looks weird, not mentioned in C++ Core Guidelines

SF: no (agreed wholeheartedly with your reasoning)

• modernize-use-override: must fix

SF: discuss: Keen to understand the value. I may just be out-of-date on this one.

• modernize-use-nullptr: must fix

SF: yes

• modernize-use-equals-delete: fix

SF: discuss: Not sure of the benefit here. Declaring without implementing (as already done) has exactly the same effect. I guess this is more explicit; is that th epoint?

• modernize-use-equals-default: fix

SF: yes

• modernize-use-emplace: fix

SF: yes

• modernize-use-default-member-init: fix

SF: Yes (see my comment in the review but really like this

• modernize-use-bool-literals: must fix

SF: YES!

• modernize-use-auto: fix

SF: yes

• modernize-return-braced-init-list: fix

SF: yes

• modernize-redundant-void-arg: ok; this is not the current style (most of the times), but recommended by C++ Core Guidelines

SF: yes (only because it is the easiest way to get consistency :-) )

• modernize-raw-string-literal: fix

SF: yes

• modernize-pass-by-value: ok

SF: no - complexity for no obvious benefit (we aren't shifting megabytes around in any of these). Happy to be convinced otherwise.

• modernize-make-shared: ok

SF: yes. If we are going to make some of the other modernize fixes, this one may as well go in.

• modernize-loop-convert: must fix; maybe needs loop variables renamed for clarity

SF: yes.

• modernize-deprecated-headers: must fix

SF: yes - but I'm nervous about cutting off folks on older platforms which was litterally the original usecase for SRecord. Chances are this worry is completely unfounded.

My wording:

• must fix: I guess everyone will agree (with the goal to avoid bugs)

• fix: useful, most people will agree

• ok: useful (but I don't have an opinion, I'm usually stuck to C++98 myself)

• no: I wouldn't do it

Adding:

• yes: Let's do this
• discuss: No strong opinion but aware I may not have all the answers either. Default is otherwise, no
• no: Preference is to avoid but please argue your point if you feel strongly

[jtxa]

Update: most messages were fixed in #32

These were agreed to be not fixed:

modernize-pass-by-value
modernize-use-trailing-return-type
modernize-use-using

So these messages are left, which can be autofixed (but may need manual changes or reviews, see comments above)

bugprone-implicit-widening-of-multiplication-result
cppcoreguidelines-init-variables
cppcoreguidelines-pro-type-member-init
modernize-loop-convert
modernize-use-default-member-init
modernize-use-equals-default
modernize-use-equals-delete
modernize-use-nullptr
modernize-use-override
readability-braces-around-statements
readability-convert-member-functions-to-static
readability-else-after-return
readability-implicit-bool-conversion
readability-inconsistent-declaration-parameter-name
readability-redundant-member-init
readability-redundant-string-init

For the current state of unfixed messages see .clang-tidy which also contains other warnings which may need to be discussed and manually fixed.