Unified code style across BlocksDS-maintained projects

[asiekierka]

clang-format exists, let's make use of it.

I have no strong opinions here, other than that:

• it might be best to broadly stick to a pre-defined style (clang-format provides LLVM, Google, Chromium, Mozilla, WebKit, Microsoft), with some changes of our own,
• the Doxygen comment style should also be unified in the process,
• // clang-format off may need to be used judiciously in areas which define registers and other such constants, as we may end up relying on unique formatting there to make it more readable for a human.

[AntonioND]

I've spent a few hours with clang-format and "Artistic Style" https://astyle.sourceforge.net/ and I'm pretty disappointed with the results of the tools in general.

clang-format:

• Pro: It can fix the format of all the elements in the code, it can even tell apart * of a multiplication from * used to declare a pointer.
• Con: However, things like a * multiplication inside a macro will be treated sometimes like a pointer. There are some issues like this with & as well.
• Con: It is very strict with line length, to the point of breaking many lines in awkward ways, and many comments, even if they overflow by one or two characters (which would normally be okay).
• Con: Different versions of the tool produce different results.
• Con: Coming up with a good configuration is way too hard. For example, with Allman braces style the extern "C" statements in all headers will cause all the other elements inside the extern "C" to be indented, and this can't be disabled manually: https://stackoverflow.com/questions/65866419/unable-to-disable-extern-indentation-with-clang-format

Artistic Style:

• Pro: It's fairly easy to configure and it's easy to relax rules so that a human can format code so that it looks nice.
• Pro: It's easy to stick to one version of the tool and have consistent results.
• Con: It doesn't understand the context of each token, so things like * and & are pretty broken.

In short... for now, let's not do this. I may spend a bit of time writing a document with some broad code style rules, and leave the rest to the discretion of the developer.

[profi200]

That reminds me of a little story from years ago when #3dsdev on EFNet was still active. Someone opened an issue regarding clang-format to add single line if(). One of the devs responded how the code styles they offer are superior and the issue got closed. So yeah, don't rely on these tools too much. It's time consuming but formatting by hand gives the best results.

[AntonioND]

Well, it looks like libnds is probably an exception. DSWiFi has taken quite a bit of work to manually change a few parts that caused clang-format to freak out. However, I have created a .clang-format file that generates code that looks basically like what I was aiming for in libnds, and I haven't had to disable it anywhere: blocksds/dswifi@5894e8d

Same for libteak: blocksds/libteak@eeb96d7

[GalaxyShard]

This is tangentially related to this issue, but I think it could help code quality & style to keep a unified set of warnings across BlocksDS. It seems like each Makefile has it's own set of enabled and disabled warnings--I figure there probably shouldn't be any disabled warnings.

[AntonioND]

This is tangentially related to this issue, but I think it could help code quality & style to keep a unified set of warnings across BlocksDS. It seems like each Makefile has it's own set of enabled and disabled warnings--I figure there probably shouldn't be any disabled warnings.

#4

There is no point in enabling any disabled warning if the occurrences of the warning aren't fixed, it would only add noise to the build. I've fixed lots of warnings and silenced the rest until they are fixed. Most of the projects are very old, and compilers didn't have as many warnings as today (and most people didn't bother enabling them anyway).

[profi200]

I think -Wall -Wextra should be on by default everywhere. It enables many useful warnings. That plus maybe some more if needed.

[AntonioND]

That's what we currently have, but we disable individual warnings in different repositories if they have instances of that warning that haven't been fixed. In most cases, I didn't fix them because I couldn't test the result properly, so I decided to not risk it.

[AntonioND]

Oh, and the idea of disabling warnings is that if there are 10 warnings and you add one while doing some change, you won't notice it. If there are zero warnings, you will.