Fix warnings from MegaLinter

[sierrafoxtrot]

Following @jtxa's wonderful additions of MegaLinter and other workflows, there are a bunch of warnings including on coding style as SRecord style doesn't fit in with the default which appears to be the Google C++ style guide. I'm starting to take a look at fixing some of these. I suspect others will also start chipping away at the list.

I figured listing work in progress here might save doubling up and treading on each other's toes. I'm also using it as a nice easy introduction to GitHub workflows for my own education.

If anyone wants to grab one of these sub-tasks, please note it

cpplint

• [ ] [whitespace/braces]. Likely going to suppress these or perhaps a subset of these. SRecord doesn't do hugging braces for example
• [ ] [build/include_order]. I like the reasoning behind this and might explore fixing these.
• [ ] [readability/casting]. Includes things like replacing c-style cast which would be a nice modernisation step.
• [ ] [runtime/int]. Not going to push back on platform independent fixed size types. We are literally manipulating bytes here!
• [ ] [runtime/*]: Most likely underlying issues that should be fixed.
• [ ] [build/header_guard]: Looks to be a style issue only. Likely to suppress the warning unless there is a concrete benefit
• [ ] [readability/namespace]: Might be a nice improvement. Considering to fix or supress
• [ ] [whitespace/newline, comma, parens, indent, etc]:To be fixed if it matches existing style
• [ ] [build/include_what_you_use] Definitely fix

[jtxa]

Before doing cpplint, I would start with clang-tidy. It has an auto-fixing feature. Please see draft PR #26 with 1 auto-fix per commit for discussion. On a first view, almost all look useful.

There's also clang-format which would be nice to auto-format the code and keep it consistent. Brace style and such things can be configured. The only problem is, that it cannot be made exactly fitting the current style. A few things cannot be configured the current way. And the other thing is, that the current source is also not consistent everywhere. When using clang-format (and checking it in QA) we can disable cpplint's whicespace/braces checks, if they cannot be configured the same way. A few years ago I have experimented with the settings. I will post another draft PR when I find them.

[sierrafoxtrot]

No worries. I'll hold of for now and check out PR #26. I love the work you are doing on this pipeline and I'm keen to contribute if I can without getting in the way.

[jtxa]

Most importantly I wanted to avoid extra work on both sides by having merge conflicts. To avoid future merge conflicts, such a big change should be done once and early, before the masses of contributors pop up ;-)

For my PR the major work is automated, so after deciding which changes shall be applied I need a ~2 day frame to do the changes and a review. During that time none of us should do bigger changes. If you agree on clang-format, then it should be done in the same PR. I hope this will be my last big PR with mass changes. As long as we communicate about that time slot, you can do any improvement you like beforehand. Just avoid spending your precious time on thing that can be auto-fixed.

I had a look into cpplints messages, I think the runtime and castings are not auto-fixed. The style checks should be disabled in favor of clang-format. The majority of runtime/int should be quite easy to fix by doing some replacements (first unsigned one, then others without prefix).
Given the fact that SRecord was developed on a 32-bit Linux, long/int was 32-bit. Today it's 64-bit, which already was the cause of problems.

[jtxa]

And about IWYU, it seems to include some tool that can also auto-fix. Perhaps a configuration is needed to get the desired order, perhaps it's easier to fix the missing ones manually.

My personal include preference is: the header corresponding to the source file, then project headers, then standard. This way the compiler automatically makes sure, that the header is self-contained.

[jtxa]

The question about cmake-format is, if you want to apply the formatting all at once or not. Some projects prefer not having any format commit and want just the changed lines correctly formatted. There is some tooling to achieve that. My personal opinion is, that after 8 years of standstill, modernizing the code to C++11 and improvements on code quality, it's the perfect time to do it once and keep it from then on.

[sierrafoxtrot]

Most importantly I wanted to avoid extra work on both sides by having merge conflicts. To avoid future merge conflicts, such a big change should be done once and early, before the masses of contributors pop up ;-)

Totally agree and love the work that you are doing on this.

For my PR the major work is automated, so after deciding which changes shall be applied I need a ~2 day frame to do the changes and a review. During that time none of us should do bigger changes. If you agree on clang-format, then it should be done in the same PR. I hope this will be my last big PR with mass changes. As long as we communicate about that time slot, you can do any improvement you like beforehand. Just avoid spending your precious time on thing that can be auto-fixed.

Totally agree. Right now, I don't have anything planned that can't be parked and rebased.

Regarding auto-fixing, Scott's Rule #0: Make the machines do the boring work (they are better at it).

I had a look into cpplints messages, I think the runtime and castings are not auto-fixed. The style checks should be disabled in favor of clang-format. The majority of runtime/int should be quite easy to fix by doing some replacements (first unsigned one, then others without prefix). Given the fact that SRecord was developed on a 32-bit Linux, long/int was 32-bit. Today it's 64-bit, which already was the cause of problems.

Sounds like a good approach, using the right tools for the appropriate task. I'd forgotten how many type choices particularly from the earlier code was platform dependent. Certainly something to fix explicitly with appropriate "modern" types.

And about IWYU, it seems to include some tool that can also auto-fix. Perhaps a configuration is needed to get the desired order, perhaps it's easier to fix the missing ones manually.

Given how rarely these change (other than major contributions like a new file format), I'd be comfortable with just a detection tool. But if you can get auto-fix working, it would be cool to see it in action.

My personal include preference is: the header corresponding to the source file, then project headers, then standard. This way the compiler automatically makes sure, that the header is self-contained.

Exactly the same for me. For extra pedantry, I also love seeing them in alphabetical order. I know for some, this is just stylistic but it shows attention to detail and frankly makes it easy to spot if you are doubling up.

The question about cmake-format is, if you want to apply the formatting all at once or not. Some projects prefer not having any format commit and want just the changed lines correctly formatted. There is some tooling to achieve that. My personal opinion is, that after 8 years of standstill, modernizing the code to C++11 and improvements on code quality, it's the perfect time to do it once and keep it from then on.

Normally I'd like to separate functional from style changes for anything that isn't just a trivial fix but in this situation, I entirely agree. These are a major overhaul and it would be silly to hold off making the code clean at the same time.

I'm happy to settle on C++11. There isn't too much in later editions that are likely to be directly of use (I had thought about doing something with CRCs and checksums using fold expressions but it isn't really that exciting). It may be worth the CXX_STANDARD to at least catch problems in older environments.

[jtxa]

Certainly something to fix explicitly with appropriate "modern" types.

First the simple fix should be done for portability reasons (maybe after or within #26). I assume that many/most/all of the uint32 (also those already there) should have been record::address_t for readability/context. But that's manual work for later.

I'm happy to settle on C++11. There isn't too much in later editions that are likely to be directly of use (I had thought about doing something with CRCs and checksums using fold expressions but it isn't really that exciting). It may be worth the CXX_STANDARD to at least catch problems in older environments.

See #27 to establish C++11 compatibility.

[jtxa]

I had vacation this week, so I could spend a good portion on SRecord. I'm happy that I managed to get the workflow running with the important warning-free check on gcc/clang. From now on I will be slower on changes and feedback. But I'll try to constantly improve code quality (I don't expect many functional changes from my side).

[sierrafoxtrot]

Thank you so much for doing all of this! It's a huge boost for the project.

[sierrafoxtrot]

See #27 to establish C++11 compatibility. Just reviewed now and looks good. A single comment but very keen to merge when you have had a moment to consider it.