Adding Clang Format file

[colincornaby]

Adding Clang format file based on Google Style. This is useful for the Metal code which wasn't really written with respect for conventions, so this will allow automatic enforcement of code style to make the pull request easier. There are two changes made to Google Style:

• Four spaces for tabbing (Google Style has two, Plasma seems to prefer 4.)
• Removing line limit (Google Style limits lines to 80 characters. Might be chaos to add that to Plasma right now.)

Clang format can be invoked using the following commands: Dry run: clang-format --dry-run -style=file plMetalDevice.cpp Apply changes in place: clang-format -style=file -i plMetalDevice.cpp

This does not automatically invoke clang-format as part of builds. There are several CMake plugins to do so (and even plugins for some IDEs.)

[Hoikas]

What is the status here?

[colincornaby]

What is the status here?

I don't think we have agreement on what the format should be, and if you run Clang-format on the entire code tree it will reformat everything. I think we'd need to have an agreement that we run this on files individually as needed - and not the entire tree.

plClient also has it's own formatting file set. I'd be fine with just closing this PR down and if people are fine with the formatting file in the client maybe we roll that up at some point.

[Hoikas]

if you run Clang-format on the entire code tree it will reformat everything

Right, the old code is all over the place in terms of code style. Ideally, what we want is for this to match the H'uru Plasma Code Style, which is mostly documented here. My suggestions were attempts to bring the clang-format rules in line with the style guideline.

[colincornaby]

if you run Clang-format on the entire code tree it will reformat everything

Right, the old code is all over the place in terms of code style. Ideally, what we want is for this to match the H'uru Plasma Code Style, which is mostly documented here. My suggestions were attempts to bring the clang-format rules in line with the style guideline.

Let me look at this again in since I'm looking at applying clang-format to the Metal code. It would be a good chance to at least apply the style to a "clean" section of the code that won't cause a bunch of needless churn.

[Hoikas]

I'm skimming through the Metal PR to see what the results look like. I still see some code style problems, but I'm not sure if clang-format can handle this, so I'm just going to make a note of it.

• I noticed that clang-format changed #endif // plMetalDevice to #endif // plMetalDevice (two spaces between the preprocessor directive and the comment). One space is sufficient.
• Multi-line arrays should have their closing curly brace on a newline. In other words,

  static const uint kFaceMapping[] = {
      1,  // kLeftFace
      0,  // kRightFace
      4,  // kFrontFace
      5,  // kBackFace
      2,  // kTopFace
      3   // kBottomFace
  };

  is good, but

  static const float fullFrameCoords[16] = {
      //first pair is vertex, second pair is texture
      -1, -1, 0, 1,
      1, -1, 1, 1,
      -1, 1, 0, 0,
      1, 1, 1, 0};

  is not good.

• Comments should have a space after //. In other words,

  // Reset the clear colors for the next pass
  // Metal clears on framebuffer load - so don't cause a clear
  // command in this pass to affect the next pass.

  is good, but

  //first pair is vertex, second pair is texture

  is not good.

[colincornaby]

Lemme take a look. I'll try to make changes on the Metal PR - and if they look good, bring them back here.

[colincornaby]

I've updated the format and also applied it to the Metal source.

(I can see there are some embarrassing copy/paste mixups with the DX or OpenGL pipelines that the format is messing with, I'll clean those up on the Metal pipeline side.)

[Hoikas]

I'm seeing some odd stuff in plMetalDevice.cpp, but that might be what you mean about copy/paste mixups. For example,

    if (src.fFlags & hsMatrix44::kIsIdent)
    {
        memcpy(dst, &matrix_identity_float4x4, sizeof(float) * 16);
    } else
    {
        memcpy(dst, &src.fMap, sizeof(matrix_float4x4));
    }

should be

    if (src.fFlags & hsMatrix44::kIsIdent) {
        memcpy(dst, &matrix_identity_float4x4, sizeof(float) * 16);
    } else {
        memcpy(dst, &src.fMap, sizeof(matrix_float4x4));
    }

or just drop the curly brackets altogether.

In plMetalFragmentShader.h:

class plMetalFragmentShader : public plMetalShader
{
   protected:

should be

class plMetalFragmentShader : public plMetalShader
{
protected:

In plMetalPipeline.cpp:

    for (auto& layer : layerStates)
    {
        layer.clampFlag = hsGMatState::hsGMatClampFlags(-1);
    }

should be:

    for (auto& layer : layerStates) {
        layer.clampFlag = hsGMatState::hsGMatClampFlags(-1);
    }

or just drop the curly brackets altogether.

[colincornaby]

Ah, I meant more stuff like DX specific comments that are still hanging out in Metal headers. I'll take a look at this stuff. Clang format should be able to enforce that...

[colincornaby]

Clang-format is currently doing something weird here. The existing rule is:

BraceWrapping:
  AfterControlStatement: MultiLine

This means if you have a multiline control statement, a brace is allowed to break. Otherwise a brace is not allowed to break. It should be enforcing that the braces are on the same line. But it's not. If I change that value to Never then it's happy to enforce all braces being on the same line as control statements.

I can change to Never. Just thought I'd note the oddity here.