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

`astyle` formatting issues #136

Closed
joesavage opened this Issue Oct 17, 2016 · 0 comments

Comments

Projects
None yet
1 participant
@joesavage
Member

joesavage commented Oct 17, 2016

I hate to be the guy opening an issue about code style in a project with maintainership issues, but it's really bugging me so here goes.

The automated formatting script in this project currently invokes astyle with the -d flag, which inserts space padding around parenthesis on the "outside" only. Presumably this option was specified in this instance to allow for conditional and iteration statements written as if (condition) and while (condition), but it also seems to be generating lines like assert ( (radix > 1) && (radix < 6)); and tw += ( (out_step >> 1) - 1);. Why? I don't know. It seems like the right side of the bracket should count as "outside" too, but apparently it doesn't.

There's another option, -xd, that's like -d but inserts padding only around the first parenthesis in a series. This fixes the previous cases, but still results in oddities such as twiddles[ (radix - 1) * j + k - 1]. Argh! And these are hardly the only examples of astyle formatting issues present within the codebase. All things considered, I'm not of the opinion that astyle is doing a good job of keeping Ne10 formatted well, and that's sort of the only thing that formatting tools are supposed to be for.

Given that it may be doing more harm than good in this sense, is it worth considering the removal of this tool from the project? Code formatting should be properly reviewed as new patches get merged, and some flexibility in how things should look can make the code more rather than less readable. Running cformatter.sh against the current master branch, I'm finding that around 1400 lines aren't currently formatted in the way that astyle wants anyway, so the current formatting rules aren't even being kept. Or, rather than no automated formatting aid, perhaps a better option here would be to have a checkpatch script similar to that of the Linux kernel, which aims to detect and report errors rather than to explicitly manipulate code to enforce strict styling rules.

Thoughts?

@joesavage joesavage closed this in 18c4c98 Nov 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment