Thoughts on improving and enforcing coding style

[AMS21]

Here are some general ideas to improve coding style.

Implementing these would generally be split into 2 steps.

1. Running the option on the entire code base.
2. Creating a GitHub workflow which run on every commit/pull request to verify it in the future.

• Replacing tabs with spaces ref Fix and enforce code style regarding spaces #1477
• Source files should be encoded with UTF-8 GitHub Actions: Add workflow to check file encoding #1480 ref
• Ensuring files don't contain windows-style line endings GitHub Actions: Add workflow to check file encoding #1480
  Should be automatically handled by .gitattributes but can't hurt to check and enforce.
• Running clang-format Update and enforce formatting for C/C++ (#1474) #1496
• Removing trailing white spaces Fix and enforce code style regarding spaces #1477
  Not a big problem but look bad IMO ref
• Ensuring files end with a new line Fix and enforce code style regarding spaces #1477
  Not required but is kind of the convention with C/C++.

To address one problem immediately. Mass formatting commits tend to mess up the git blame. But fortunately for this very problem the --ignore-rev option was created which allows you to ignore certain commits and even load these from a file.

And GitHub supports this commit ignoring out of the box see this.

So all of these mass formatting commits would be added to the .git-blame-ignore-revs file and should not show up in the git blame. For local use you might have to set it up manually like this.

git config --local blame.ignoreRevsFile ".git-blame-ignore-revs"

What are the community thoughts on this?

I have implemented all of these options in past so it shouldn't be a big problem.

[Xottab-DUTY]

Agree on everything, except details about clang-format'ing the code:
We had a discussion in #278 about the code style modification, and I also suggested to change code style for the entire code base to snake_case. The team didn't like the latter, and first kinda did end in nothing.

So, now, we have a very permitting mixed code style: since the original code already has mixed snake_case and PascalCase and Hungarian notation, we allow every developer to do the same. Generally, sticking to the code style of the part of the code, or the code style of the file you are working on. But developer is also allowed to stick to the his personal preference. It just should not like an alien to the existing X-Ray code base =)
Because of that, clang-format config should be modified to allow everything above and only enforce few things:

1. Always using spaces (except for comments, ignore tabs in C/C++ comments)
2. Opening bracket { should be always on a new line
3. Removing trailing whitespace (for the GitHub action case, it should allow it for existing code, but disallow it for new code)
   (I will added more items to this list later as I recall them)

Replacing tabs with spaces

This was done in 2017 with clang-format, but since then a little number of tabs could remain or even be added. It's good to check the code again.

Source files should be encoded with UTF-8

The same. All sources were converted to UTF-8 somewhere in 2017, but it's good to check it again and even enforce that using GitHub workflow.

[Xottab-DUTY]

@OpenXRay/xray-contributors

[AMS21]

So now the hard part: clang-format

We could run clang-format on the entire code base but I don't think this would be a good idea the diff is absolutely massive and a lot of the original code is actually formatted in a nice readable way just in a bit of a different style.

Instead I would suggest running the clang-format-diff tool on commits and pull requests which would only check lines that were actually changed. So we would only enforce our formatting rules on new code and leave the old as is.

Now for the actually formatting rules there are some which I liked to at least discuss.

• AlignConsecutiveAssignments and AlignConsecutiveDeclarations
  These are currently set to false but in my opinion these make code a lot more readable. These can also be configured to work across lines and comments. There is also AlignConsecutiveMacros but I don't see it as that important.

• AllowShortEnumsOnASingleLine
  Should in my opinion be set to false.

• AllowShortLambdasOnASingleLine
  Because short functions are allowed on a single line I think lambdas should also be allowed.

• AttributeMacros
  Are there any attribute like macros we use inside the code base?

• BraceWrapping
  We break on almost everything which is consistent but not on case labels. So we would get code like this:

swtich (value)
{
    case A: {
        ...
        break;
    }
}

Is this intended?

Also BeforeWhile should should probably also be set to true.
Also theres SplitEmptyFunction, SplitEmptyRecord and SplitEmptyNamespace which when set to false allows them to be put on a single line.

• BreakAfterAttributes
  I've seem code formatted like this already but can also be set to leave which would basically ignore this option.

• BreakConstructorInitializers
  There is BreakConstructorInitializersBeforeComma inside the .clang-format but this is not a real option.

• CompactNamespaces
  Looking at the code I think this should be set to false

• EmptyLineAfterAccessModifier
  From what I've seen either Nerver or Leave

• EmptyLineBeforeAccessModifier
  Very similar I guess either Always or Leave

• FixNamespaceComments
  I don't have a strong opinion either way. It can be nice to see which closing brace belongs to which namespace but I don't think we use namespaces that much anyways.

• IndentPPDirectives
  Can be used to easier see where which #if, #else, etc. block ends. But I don't think this is a very common style inside the code base. Should be set together with PPIndentWidth which should then probably also 4.

• InsertBraces
  I think this should be set to false to align with the current code style. This allows code like this:

if (x)
    return;

instead of

if (x)
{
    return;
}

and also consider setting RemoveBracesLLVM which would not allow the second example.

• InsertNewlineAtEOF
  Since we already enforce that it only makes sense to enable this in .clang-format as well.

• QualifierAlignment
  I think most of the code uses Left alignment.

• QualifierOrder
  Could be interesting and make the make the code more consistent. But might also be a bit too pedantic saying static const int i = 42; is bad and must be const static int i = 42; for example.

• ReferenceAlignment
  Similar to QualifierAlignment I think we use Left alignment.

• RemoveParentheses
  Not sure if this is actually needed or would provide more problems than it solves. Also gcc has the -Wparentheses warning which should catch these exact cases.

• SortIncludes
  I like to sort my includes personally since it makes reading the include list easier like seeing everything which comes from one module close together and very easily spotting duplicate includes. But I also see the point in rejecting code which has not sorted its includes as a bit pedantic and it is currently disable in the .clang-format file.

• SpaceAfterLogicalNot SpaceAfterTemplateKeyword SpaceBeforeCaseColon SpaceBeforeSquareBrackets SpaceInEmptyBlock SpacesInParens
  I think these should all be set to false.

• SpaceAroundPointerQualifiers
  I think before makes the most sense here.

• SpaceBeforeCpp11BracedList
  Honestly not sure about this one.

• SpaceBeforeCtorInitializerColon
  I think this should be set to true.