Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

workflows/lint: add clang-format on changed files #15132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kasper93
Copy link
Contributor

The entire codebase is not ready to be clang-formatted and probably never will be, but we can at least check if the changes in new pull requests follow our coding style.

Copy link

github-actions bot commented Oct 20, 2024

Download the artifacts for this pull request:

Windows
macOS

.github/workflows/lint.yml Outdated Show resolved Hide resolved
@kasper93 kasper93 force-pushed the clang-format branch 2 times, most recently from 4120eca to 4347812 Compare October 21, 2024 18:09
@sfan5
Copy link
Member

sfan5 commented Oct 29, 2024

Has anyone tried to verify how well this matches our code style by running it on existing files?
If not I will.

@sfan5
Copy link
Member

sfan5 commented Nov 6, 2024

Here's it applied to a few big files: sfan5@bf400f2

As I feared, either the config is not fitting or clang-format is just too opinionated to be applied to all new or changed code.
In my experience it's the latter and you either up with a sea of // clang-format on|off or you submit yourself to the linter compromising on readability.

just look at this

    .defaults =
        &(const struct demux_opts){
                                  .enable_cache       = -1,  // auto
            .max_bytes          = 150 * 1024 * 1024,
                                  .max_bytes_bw       = 50 * 1024 * 1024,
                                  .donate_fw          = true,
                                  .min_secs           = 1.0,
                                  .min_secs_cache     = 1000.0 * 60 * 60,
                                  .seekable_cache     = -1,
                                  .index_mode         = 1,
                                  .mf_fps             = 1.0,
                                  .access_references  = true,
                                  .video_back_preroll = -1,
                                  .audio_back_preroll = -1,
                                  .back_seek_size     = 60,
                                  .back_batch =
                {
                    [STREAM_VIDEO] = 1,
                    [STREAM_AUDIO] = 10,
                }, .meta_cp = "auto",
                                  },

@kasper93
Copy link
Contributor Author

kasper93 commented Nov 7, 2024

@sfan5: Fixed. Any other suggestions?

The entire codebase is not ready to be clang-formatted and probably
never will be, but we can at least check if the changes in new pull
requests follow our coding style.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants