update clang-format and format some files as test #4694
Conversation
src/common/libsubprocess/server.c
Outdated
| if (flux_respond_pack (rex->s->h, rex->msg, "{s:s s:i}", | ||
| "type", "complete", | ||
| "rank", rex->s->rank) < 0) | ||
| "rank", rex->s->rank) | ||
| < 0) |
There was a problem hiding this comment.
Looking back at this now, I'm wondering what was left to adjust to make this good enough. The thing I remember trying to work around is the break before trailing < 0 in if statements, which is why this is a comment on these lines as an example. Oddly though, looking back at it I kinda don't hate it. It makes it visually obvious what's being compared here, it can be avoided by wrapping in parentheses, and if we're happy with the rest I'm not sure it's worth blocking on. @garlick, @grondo, @chu11, thoughts?
There was a problem hiding this comment.
I really don't like the break before < 0 which is why I suppose we don't do it in our existing formatting.
There was a problem hiding this comment.
Odd as it seems, we might also be able to address it with a bit of scripting. It's a very specific spot, and a vim script of %g/^\s+\(\!=|==|<|>\) \d.{0,5}$/normal kJ seems to fix it 100% of the time. The only downside is that it would mean in a few cases (less than a dozen total I think) we'll end up with lines up to 5 chars longer than the limit set in the clang-format file. I'm going to throw that into a wrapper script, maybe convert it to sed, and throw up the update.
There was a problem hiding this comment.
Updated versions of the test files pushed, I made a small mistake so there's a blank line at the beginning of the files, but the trailing comparison operator is taken care of. Need to figure out how to make it reasonably nice to use this, awk makes the process a bit slow, but does that otherwise take care of it?
Codecov Report
@@ Coverage Diff @@
## master #4694 +/- ##
=======================================
Coverage 83.38% 83.38%
=======================================
Files 413 413
Lines 69430 69408 -22
=======================================
- Hits 57892 57877 -15
+ Misses 11538 11531 -7
|
|
Huh, one other option I didn't know existed (not sure it did last I checked), but that makes things more consistent and actually works a lot like the black format for arguments would be to switch AlignAfterOpenBracket from Align to BlockIndent. That would mean that instead of aligning all function arguments to the indent of the open parentheses, they would all get the continuation indent, which looks like this: if ((port1 = get_port (triple->range)) < 0 || (port2 = get_port (triple->range)) < 0
┆ || !(arr = json_pack ("[I, I]", port1, port2))
┆ || flux_jobtap_event_post_pack (
┆ ┆ ┆ triple->plugin,
┆ ┆ ┆ triple->jobid,
┆ ┆ ┆ "cray_port_distribution",
┆ ┆ ┆ "{s:O, s:I}",
┆ ┆ ┆ "ports",
┆ ┆ ┆ arr,
┆ ┆ ┆ "random_integer",
┆ ┆ ┆ random
┆ ┆ ) < 0
┆ || flux_jobtap_job_aux_set (
┆ ┆ ┆ triple->plugin,
┆ ┆ ┆ triple->jobid,
┆ ┆ ┆ CRAY_PALS_AUX_NAME,
┆ ┆ ┆ arr,
┆ ┆ ┆ (flux_free_f)json_decref
┆ ┆ ) < 0) {
┆ if (arr)
┆ ┆ json_decref (arr);
┆ prolog_status = 1;
┆ flux_log_error (
┆ ┆ h,
┆ ┆ PLUGIN_NAME
┆ ┆ ": "
┆ ┆ "Failed to post ports to job"
┆ );
┆ goto cleanup;
}I'm oddly fond of this and interested in what everyone else thinks. Having gotten used to it with black (and how it avoids extra diff gak in argument lists, literals and so-forth) it works for me more than it once would have, and it naturally keeps the With the current value it looks like this (with the fixup script): if ((port1 = get_port (triple->range)) < 0 || (port2 = get_port (triple->range)) < 0
┆ || !(arr = json_pack ("[I, I]", port1, port2))
┆ || flux_jobtap_event_post_pack (triple->plugin,
┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ triple->jobid,
┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ "cray_port_distribution",
┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ "{s:O, s:I}",
┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ "ports",
┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ arr,
┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ "random_integer",
┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ random) < 0
┆ || flux_jobtap_job_aux_set (triple->plugin,
┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ triple->jobid,
┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ CRAY_PALS_AUX_NAME,
┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ arr,
┆ ┆ ┆ ┆ ┆ ┆ ┆ ┆ (flux_free_f)json_decref) < 0) {
┆ if (arr)
┆ ┆ json_decref (arr);
┆ prolog_status = 1;
┆ flux_log_error (h,
┆ ┆ ┆ ┆ ┆ PLUGIN_NAME
┆ ┆ ┆ ┆ ┆ ": "
┆ ┆ ┆ ┆ ┆ "Failed to post ports to job");
┆ goto cleanup;
} |
Update clang-format using some options from versions up to 11, and format some files with version 12 so we can see what they look like. This is not really meant to be merged, but to pick a few large-ish files with various code patterns so we can figure out if this is what we want.
The only thing that really stood out to me in here is the
ok ()and similar functions/macros seem like they are almost always formatted in a non-standard way. Should those go in with the pack/unpack functions so they're left alone?