obsproject / obs-plugintemplate

GNU General Public License v2.0
285 stars 133 forks source link

Update .clang-format to match obs-studio project #82

Closed Susko3 closed 8 months ago

Susko3 commented 1 year ago

Updated to https://github.com/obsproject/obs-studio/blob/93f5b45be8c8b91199c52cf7a0ab59d8c3a08760/.clang-format#L208

Old description:

Description

Fixes the .clang-format file to conform with https://clang.llvm.org/docs/ClangFormatStyleOptions.html.

Motivation and Context

I like to use CLion for C++ development, so having this file error out was bugging me.

image

How Has This Been Tested?

Opened the files of interes in CLion and checked the 'problems' window. Also, I used CLion 'Reformat code' action to reformat the code according to the file.

Types of changes

Checklist:

Susko3 commented 1 year ago

I'm not really sure what clang-format version CLion is validating against, but I would presume it's the latest one at the time of the release.

did you ensure that the schema check was done based on the schema used by clang-format-13?

Why clang-format 13 specifically? Also I'm not really sure how to verify the schema programmatically (other than using CLion). Looking at the command line options, --verbose or --dump-config (to dump the parsed, in-memory values) seem like the only options that could work. I don't have clang-format setup on my machine so can't really test it, since you seem to already have it setup, could you try validating schema?

But if the schema is different and incompatible between versions, it might regress and show warnings in CLion.

PatTheMav commented 1 year ago

We currently default to clang-format version 13.0.1 specifically, because that's the version shipped with Visual Studio 17 2022 and code style guidelines for obs-studio (and be extension the plugin template) are based on what Microsoft ships there as a baseline.

Version 14 and above do different formatting decisions, so code formatting with clang-format 14.0.6 will not pass a formatting check on CI (for which we use version 13.0.1).

And as I mentioned, the same configuration setting is upgraded between clang versions - on version 13 it might take a boolean value, on version 14 it might now take a specific enum value.

So changing the file with a linter based on the schema of a more recent version might silence the linter, but has the potential to break with a specific version of clang-format.

Susko3 commented 1 year ago

We currently default to clang-format version 13.0.1 specifically, because that's the version shipped with Visual Studio 17 2022

Latest VS Community 2022 (17.6.4) is using clang-format version 15.0.1 on my machine. The one used by Rider (same devepoler as CLion) is version 15.0.2. If they use semver for their version number, the scheme shouldn't have changed.

PS C:\> & "C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\Llvm\bin\clang-format.exe" --version
clang-format version 15.0.1

Version 14 and above do different formatting decisions, so code formatting with clang-format 14.0.6 will not pass a formatting check on CI (for which we use version 13.0.1).

Does it actually do different formatting decisions, or does it fail to parse the .clang-format made for version 13 and use defaults that don't match?

Susko3 commented 1 year ago

I find the current situation with .clang-format very strange, so I've made a discussion in the main repo: https://github.com/obsproject/obs-studio/discussions/9231.

PatTheMav commented 1 year ago

Does it actually do different formatting decisions, or does it fail to parse the .clang-format made for version 13 and use defaults that don't match?

It does.

clang-format-13:

    inline QMainWindow *GetMainWindow() const { return mainWindow.data(); }
[..]
        if (info->type == OBS_SOURCE_TYPE_FILTER)
        private
        = true;
    }

[..]
    struct TakeOwnership {
    };
[..]
       }
    };

}

clang-format-14:

    inline QMainWindow *GetMainWindow() const
    {
        return mainWindow.data();
    }
[..]
        if (info->type == OBS_SOURCE_TYPE_FILTER)
        private = true;

    }
[..]
    struct TakeOwnership {};

[..]
       }
    };

}

clang-format-15:

    inline QMainWindow *GetMainWindow() const
    {
        return mainWindow.data();
    }
[..]
        if (info->type == OBS_SOURCE_TYPE_FILTER)
        private = true;

    }
[..]
    struct TakeOwnership {};

[..]
        }
    };

}  // namespace std

Some of those changes are simply bug fixes in clang-format-14 for issues in 13, but the code will effectively have formatting changes just by upgrading the version of the formatter.

RytoEX commented 1 year ago

If they use semver for their version number, the scheme shouldn't have changed.

Past experience demonstrates that this is not the case. They have made changes in patch releases that produce different formatting output.

RytoEX commented 11 months ago

This repo should use the .clang-format settings from https://github.com/obsproject/obs-studio/pull/9235. This can be done by updating this PR or closing this and opening a new one.

PatTheMav commented 9 months ago

Please squash commits and update commit as well as PR title to reflect that this is just a clang-format update.

PatTheMav commented 9 months ago

While the commit message title is correct now, the additional text is not fine: The URL breaks column limits and the personal remark should be removed.