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",
                                  },

@sfan5 sfan5 removed their request for review November 6, 2024 19:43
@kasper93
Copy link
Contributor Author

kasper93 commented Nov 7, 2024

@sfan5: Fixed. Any other suggestions?

@sfan5 sfan5 self-requested a review November 7, 2024 12:56
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.
@sfan5
Copy link
Member

sfan5 commented Dec 15, 2024

here's it again: sfan5@c80a70c
most of the changes are fine, however the obsessive "let's align everything" is weird.
also why this?

-    in->d_thread->params = params; // temporary during open()
+    in->d_thread->params = params;  // temporary during open()

and it still makes a mess of structs

    *ctx                   = (struct mp_cmd_ctx){
                          .mpctx              = mpctx,
                          .cmd                = talloc_steal(ctx, cmd),
                          .args               = cmd->args,
                          .num_args           = cmd->nargs,
                          .priv               = cmd->def->priv,
                          .abort              = talloc_steal(ctx, abort),
                          .success            = true,
                          .completed          = true,
                          .on_completion      = on_completion,
                          .on_completion_priv = on_completion_priv,
    };

another funny example where it somehow decided to not align:

-        {"samplerate",      SUB_PROP_INT(mp_aframe_get_rate(fmt))},
-        {"channel-count",   SUB_PROP_INT(chmap.num)},
-        {"channels",        SUB_PROP_STR(mp_chmap_to_str(&chmap))},
-        {"hr-channels",     SUB_PROP_STR(mp_chmap_to_str_hr(&chmap))},
-        {"format",          SUB_PROP_STR(af_fmt_to_str(mp_aframe_get_format(fmt)))},
-        {0}
+        { "samplerate", SUB_PROP_INT(mp_aframe_get_rate(fmt)) },
+        { "channel-count", SUB_PROP_INT(chmap.num) },
+        { "channels", SUB_PROP_STR(mp_chmap_to_str(&chmap)) },
+        { "hr-channels", SUB_PROP_STR(mp_chmap_to_str_hr(&chmap)) },
+        { "format", SUB_PROP_STR(af_fmt_to_str(mp_aframe_get_format(fmt))) },
+        { 0 }

@kasper93
Copy link
Contributor Author

kasper93 commented Dec 27, 2024

also why this?

Why not? It's somewhat common to separate comments.

most of the changes are fine, however the obsessive "let's align everything" is weird.

To align or not to align, that's the question. I find aligned assignment more readable and in some cases forces to split unrelated variables in different blocks, which is also a good thing. Alternative would be to not align, so it would remove alignment from if existed previously.

and it still makes a mess of structs

It aligns it correctly. Add a blank line, before *ctx = ..., for readability.

another funny example where it somehow decided to not align:

This is different alignment. Each type can be configurable and I disabled it for structs, because neither left, nor right alignment looks good for our huge command/options structs.

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