mpv-player
workflows/lint: add clang-format on changed files
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.
Download the artifacts for this pull request:
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Has anyone tried to verify how well this matches our code style by running it on existing files? If not I will.
Here's it applied to a few big files: https://github.com/sfan5/mpv/commit/bf400f2bd6c02b25a1e9d9c8ccd9d05f3d3bab93
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",
},
here's it again: https://github.com/sfan5/mpv/commit/c80a70c4fae449eaf1f858699d10ca95617af7fa 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 }
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.
Late to this, but is it possible to just not touch the alignments at all? It seems too opinionated and inconsistent to try and enforce that imo. The rest seems OK.
Why not? It's somewhat common to separate comments.
Personally I find one space nicer, but also this isn't just part of our code style. contribute.md talks about "K&R style" which defines brace placement but not much of anything else. The code examples all use a single space and this variant is overwhelmingly more popular in current code.