Adopt clang-format

[abaire]

#648 added a .clang-format file to the cxbe tool in an attempt to cut down contributor/maintainer time spent on format nitpicking.

Ideally this approach could be adopted broadly (using the style from the nxdk or winapi libs #499 (comment)) and could be enforced through githooks or a CI step verifying that PRs do not introduce non-conformant code.

[mborgerson]

I'm in favor of both clang-format adoption and enforcing formatting in CI. My personal code style preferences differ from the cxbe tool style, and Windows style conventions broadly, but having a common rule set and tools to format code automatically saves valuable contributor time.

I merged #648 due to relative isolation and low impact, but picking the style that we may apply to all of nxdk, and whether or not we enforce the style in CI should be something about which maintainers reach consensus.

Formatting differences between the libraries/tools dependencies are inevitable, so for specific tools, like cxbe, having individual .clang-formats is reasonable.

Running clang-format on the code base can create a lot of churn, so we may want to use a .git-blame-ignore-revs file to hide the big format changes when blaming.

It is sometimes controversial, but its also worth considering auto-formatting in CI. pre-commit can do this, for example.

[thrimbor]

A CI format checking step would be my preferred solution. I worked on creating a format spec file for nxdk for a bit quite a while ago: https://github.com/thrimbor/nxdk/tree/clang-format
Iirc it wasn't possible to specify the whitespace before the opening parenthesis on function declarations, but that was just a preference of mine, if we can bring in more consistency and automatic formatting I'm more than open to having that changed.

The nxdk and winapi libraries should already have matching coding styles, and it's the one I'd prefer for nxdk. We'll have to exclude external dependencies from the checks ofc.

[RadWolfie]

I'm against having clang-format inspection in CI as this would block possible new code changes may break some things when CI is ran on its own. An example of having clang-format inspection from pull request workflow can be found here which is easier for pull request authors to fix their end of issue(s), if any, and then re-format if haven't done so. Either by git-hook or via IDE's format documents. Main IDEs already support .clang-format anyway.

It is sometimes controversial, but its also worth considering auto-formatting in CI. pre-commit can do this, for example.

Having auto-formatting in CI isn't friendly to pull request author's ongoing changes and fixups. Therefore require them to force push their branch every time. The author of pull requests should be responsible for formatting their code.

P.S. I don't see C/C++ as supported language in pre-commit's website?

Iirc it wasn't possible to specify the whitespace before the opening parenthesis on function declarations, but that was just a preference of mine, if we can bring in more consistency and automatic formatting I'm more than open to having that changed.

Had you looked at SpaceBeforeParensOptions option which is featured in v14? Although, I don't really like having a space between function's definition name and its parentheses. 🤷

The nxdk and winapi libraries should already have matching coding styles, and it's the one I'd prefer for nxdk. We'll have to exclude external dependencies from the checks ofc.

From mentioned example of pull request workflow above, it is possible to exclude external dependencies via DoozyX/clang-format-lint-action Action. Yet could be already excluded by not doing recursive module option in pull request workflow. According to lib folder, it's not ideal to have mixed of external libraries and internal libraries together in same folder as this will conflict with .clang-format file within lib folder. Which will only happen if external library doesn't have their own .clang-format file. One possible alternative without fix the lib folder is to create symbolic link of .clang-format shortcut per internal library folder to a single .clang-format file in order to handle it all. (NOTE: Windows devs may could have issue with git's symbolic link if not yet configured.)

P.S. I'm not against cxbe's individual .clang-format file as it appears to be using different format than others.

[abaire]

I'm against having clang-format inspection in CI as this would block possible new code changes may break some things when CI is ran on its own. An example of having clang-format inspection from pull request workflow can be found here which is easier for pull request authors to fix their end of issue(s), if any, and then re-format if haven't done so. Either by git-hook or via IDE's format documents. Main IDEs already support .clang-format anyway.

In my opinion the important thing is that the auto-format is automatically enforced and that code reviewers don't have to waste time pointing out style nits that would be taken care of by the tool anyway. Any solution that performs the cleanup prior to reviewers having to deal with it seems fine to me.

Having auto-formatting in CI isn't friendly to pull request author's ongoing changes and fixups. Therefore require them to force push their branch every time. The author of pull requests should be responsible for formatting their code.

Relying on humans to go out of their way to format things will always fail and adds another round trip to the process since reviewers now also have to ask contributors to run clang-format and re-push (or blindly review style-breaking code and trust that it'll be auto-fixed into something reasonable). I also think force pushing squashed changes is already the norm in this repo (e.g., #499 (comment)) so enforcing early probably isn't much of a change from the status quo.

P.S. I don't see C/C++ as supported language in pre-commit's website?

pre-commit is just a plugin layer on top of normal git hooks, it doesn't care about the actual content of the repo. The supported languages are the list of languages that may be used to write the hooks themselves, where Python or shell are likely much easier to write and maintain. That said, there tons of existing hooks (including one for clang-format) so ideally custom hooks would rarely be needed.

Iirc it wasn't possible to specify the whitespace before the opening parenthesis on function declarations, but that was just a preference of mine, if we can bring in more consistency and automatic formatting I'm more than open to having that changed.

Had you looked at SpaceBeforeParensOptions option which is featured in v14? Although, I don't really like having a space between function's definition name and its parentheses. 🤷

One big benefit and problem w/ clang-format is that it's a moving target with new options like this added regularly. Some minimum version would probably need to be chosen and communicated so that user-initiated reformats don't fight with repo-side ones where there's a mismatch in supported opts.

From mentioned example of pull request workflow above, it is possible to exclude external dependencies via DoozyX/clang-format-lint-action Action. Yet could be already excluded by not doing recursive module option in pull request workflow. According to lib folder, it's not ideal to have mixed of external libraries and internal libraries together in same folder as this will conflict with .clang-format file within lib folder. Which will only happen if external library doesn't have their own .clang-format file. One possible alternative without fix the lib folder is to create symbolic link of .clang-format shortcut per internal library folder to a single .clang-format file in order to handle it all. (NOTE: Windows devs may could have issue with git's symbolic link if not yet configured.)

+1 to moving the third party submodules out of lib and into a peer directory, if that's feasible/reasonable.

[mborgerson]

Sounds like we are mostly in agreement. Let's move forward on adopting clang-format and adding a check step (whenever someone is motivated to work on this, that is)