clang-format: tweaks #8655

hjelmn · 2021-03-19T03:09:50Z

This commit makes the following changes:

Disallow single-line functions. This matches the dominant style used
in Open MPI.
Increase the penalty for breaking on assigment slightly higher than
breaking before first argument. That will ensure that long assignments
of the form foo = bar(baz) will break after the = before trying to
break after the open-paren.
Break before binary operators. This is a change in style that I will
revert if there is any push back. This causes long statements to break
before the binary operator not after it. This is the currently
recommended practice.

Signed-off-by: Nathan Hjelm [email protected]

bosilca

All looks good. I actually like the operator to start the new lines, I agree it improves the readability. The breaking between the function name and the list of argument I think it's just weird, and we should try avoid if possible.

gpaulsen · 2021-03-19T14:43:38Z

@hjelmn Can you please document how to run clang format and the new clang-tidy tools in the ompi-wiki?

I see some instructions here: #8551, but since then there's been mention of this other clang-tidy tool.
Please also include version info, as @awlauria tried it locally and ran into some errors that you didn't.
Thanks.

hjelmn · 2021-03-19T14:55:16Z

@gpaulsen Sure. I intend to upload a script and set of configuration files for clang-tidy shortly. clang-tidy has some bugs with some of the checkers (duplicate branch detection screws up the code) so I am testing them out one-by-one now. Adding single-statement conditional braces works well.

hjelmn · 2021-03-19T21:23:36Z

I did find more errors in OMPI BTW. I plan to patch those in the clang-format ompi branch.

jsquyres · 2021-03-20T21:17:11Z

Once you settle on a final .clang-format file and clean the entire tree, see jsquyres#3 for an example how this could be run in CI.

hjelmn · 2021-03-22T18:08:22Z

I think I resolved the remaining formatting issues (to the degree possible). Made one additional tweak. clang-format was putting all the arguments to a function/macro on the next line if it couldn't fit them on the same line as the function or macro. This could lead to breaking before the first argument which is undesirable.

awlauria · 2021-03-22T18:32:19Z

What version of clang-format are you using?

When I try this locally, I get:

clang-format --style=file --verbose ./opal/include/opal/sys/atomic.h 
Formatting ./opal/include/opal/sys/atomic.h
YAML:39:18: error: invalid boolean
AlignOperands:   Align
$. clang-format --version
clang-format version 9.0.1 (Red Hat 9.0.1-2.module+el8.2.0+5494+7b8075cf)

jsquyres · 2021-03-22T18:43:44Z

When I was testing the GitHub Action with clang-format, I had to be sure to use clang-format v11. v10 complained about a bunch of options in the config file.

hjelmn · 2021-03-23T14:38:01Z

@jsquyres Which options? They might be ones we only use the defaults for.

hjelmn · 2021-03-23T14:40:20Z

@awlauria Ok, I see. llvm 9 is a bit long in the tooth (~1.5 years old) now but that is still relatively fresh. I want at least v10 to work if possible. The homebrew version is v11.0.0. Let me see if I can get v9 and v10 installed to at least evaluate.

hjelmn · 2021-03-23T14:43:19Z

Homebrew has v8 and v11. Let me look at what v8 has.

I am actively working on getting patches in for the next version of clang-format (v13). If there is anything we want in particular let me know.

jsquyres · 2021-03-23T14:48:05Z

@jsquyres Which options? They might be ones we only use the defaults for.

The only one I recall offhand was "Align", where our value was "Align", and the only allowable options (IIRC) were true and false. After I ran into 2-3 cases like this, I realized that I must be doing it wrong, and checked the versions, noticed that I was running v10 and v11 was available, ... yadda yadda yadda.

awlauria · 2021-03-23T14:49:53Z

@hjelmn that's probably fine. We can figure out how to upgrade it on our systems with the decided version, I wouldn't spend too many cycles on that. Thanks.

hjelmn · 2021-03-23T15:15:45Z

@awlauria, @jsquyres I can set AlignOperands to true. That works the same as Align in 11+. They added some additional values which changed it from a bool to an enum. When I dumped the config it got Align because of this change.

AlignOperands (OperandAlignmentStyle)
   If true, horizontally align operands of binary and ternary expressions.

OAS_Align (in configuration: Align) Horizontally align operands of binary and ternary expressions.

This commit makes the following changes: - Disallow single-line functions. This matches the dominant style used in Open MPI. - Increase the penalty for breaking on assigment slightly lower than breaking before first argument. That will ensure that long assignments of the form foo = bar(baz) will break after the = before trying to break after the open-paren. - Break before binary operators. This is a change in style that I will revert if there is any push back. This causes long statements to break before the binary operator not after it. This is the currently recommended practice. - Don't allow all arguments on the next line. This may cause a break after an open paren which is not desired. - Change AlignOperands to true (synonym for Align in v11+) to support clang-format v10. Signed-off-by: Nathan Hjelm <[email protected]>

hjelmn · 2021-03-23T15:17:54Z

Done. Should work with clang-format v10 now.

hjelmn · 2021-03-23T15:32:31Z

@awlauria I would recommend the latest version you can install of either 10 or 11. That way we get the newest set of bug fixes. Once 13 is out I would like to switch to that but it will be awhile.

awlauria · 2021-03-23T15:52:01Z

@hjelmn thanks! I'll look into getting v11 installed on our machines.

ibm-ompi · 2021-03-23T16:31:37Z

The IBM CI (PGI) build failed! Please review the log, linked below.

Gist: https://gist.github.com/e546e2c4d86b82f7d1e43f5dd26f81c2

awlauria · 2021-03-23T16:44:07Z

bot:ibm:pgi:retest

ibm-ompi · 2021-03-23T17:58:54Z

The IBM CI (PGI) build failed! Please review the log, linked below.

Gist: https://gist.github.com/aaefe9301fca2874253bacabd1f67f63

hjelmn · 2021-03-23T18:07:05Z

Lets not hit CI again for this :). It is hurting enough already.

awlauria · 2021-03-23T19:18:54Z

Yeah, seems the ibm ci is having a bad day. this ready to merge?

hjelmn added Target: main Target: v5.0.x labels Mar 19, 2021

hjelmn requested review from jsquyres, bosilca and bwbarrett March 19, 2021 03:09

bosilca approved these changes Mar 19, 2021

View reviewed changes

hjelmn force-pushed the clang_format_redux branch from 85b9b75 to 4d6bf07 Compare March 22, 2021 14:42

hjelmn force-pushed the clang_format_redux branch from 4d6bf07 to 8c72920 Compare March 23, 2021 15:17

hjelmn merged commit 90374ec into open-mpi:master Mar 23, 2021

clang-format: tweaks #8655

clang-format: tweaks #8655

Uh oh!

Conversation

hjelmn commented Mar 19, 2021

Uh oh!

bosilca left a comment

Choose a reason for hiding this comment

Uh oh!

gpaulsen commented Mar 19, 2021

Uh oh!

hjelmn commented Mar 19, 2021

Uh oh!

hjelmn commented Mar 19, 2021

Uh oh!

jsquyres commented Mar 20, 2021

Uh oh!

hjelmn commented Mar 22, 2021

Uh oh!

awlauria commented Mar 22, 2021

Uh oh!

jsquyres commented Mar 22, 2021

Uh oh!

hjelmn commented Mar 23, 2021

Uh oh!

hjelmn commented Mar 23, 2021

Uh oh!

hjelmn commented Mar 23, 2021

Uh oh!

jsquyres commented Mar 23, 2021

Uh oh!

awlauria commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hjelmn commented Mar 23, 2021

Uh oh!

hjelmn commented Mar 23, 2021

Uh oh!

hjelmn commented Mar 23, 2021

Uh oh!

awlauria commented Mar 23, 2021

Uh oh!

ibm-ompi commented Mar 23, 2021

Uh oh!

awlauria commented Mar 23, 2021

Uh oh!

ibm-ompi commented Mar 23, 2021

Uh oh!

hjelmn commented Mar 23, 2021

Uh oh!

awlauria commented Mar 23, 2021

Uh oh!

Uh oh!

awlauria commented Mar 23, 2021 •

edited

Loading