Add a .clang_format file for Open MPI #8551

hjelmn · 2021-03-03T20:20:06Z

This file works with clang-format --style=file to reformat code to match the
style used in Open MPI. This type includes:

No tabs. They are not recommended for Open MPI and can often screw up the
formatting.
Tab depth: 4. This is what is used throughout the Open MPI code base.
Max column width: 100. There currently is no standard for Open MPI but we
should aim to use something reasonable.
Braces following function definitions occur un-indented on the following
line.
Braces following other control statements occur on the same line as the
control statement.
Spaces always before open parentheses for control statements (if, while, do,
etc). This is common across the code base but not consistent. No spaces
after function names, and function or macro calls.
Align consecutive macro definitions.

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

hjelmn · 2021-03-03T20:20:44Z

If this seems like a good idea lets iterate until the style is what we want. Then I would recommend running clang-format on the entire code base.

hjelmn · 2021-03-03T20:23:02Z

Should mention that this only is used if you run clang-format --style=file -i <file>. The semantics of --style=file is the first .clang_format that is hit in the current dir or a parent directory.

bosilca · 2021-03-03T20:52:31Z

This looks like indent on steroids. Nice !

hjelmn · 2021-03-03T21:33:11Z

There is also a git tool for this:

git clang-format --style file

It can fix fix, show formatting fixes, etc for currently edited files or a commit.

Example:

git clang-format --style file -q --extensions c,h --diff 2fb5f551b75878d0617f5392d4dff860fd057181 9b2ed1e87c80bb8034c49a4bf12a42ddc2e4940f
diff --git a/ompi/mca/osc/rdma/osc_rdma_lock.h b/ompi/mca/osc/rdma/osc_rdma_lock.h
index 2d3d88b2af..f4de66977a 100644
--- a/ompi/mca/osc/rdma/osc_rdma_lock.h
+++ b/ompi/mca/osc/rdma/osc_rdma_lock.h
@@ -89,7 +89,7 @@ static inline int ompi_osc_rdma_btl_fop (ompi_osc_rdma_module_t *module, struct
                                            pending_op->op_frag->handle, (void *) pending_op, NULL, OPAL_SUCCESS);
         } else {
             /* need to release here because ompi_osc_rdma_atomic_complete was not called */
-            OBJ_RELEASE(pending_op);
+            OBJ_RELEASE (pending_op);
         }
     } else if (wait_for_completion) {
         while (!pending_op->op_complete) {

hjelmn · 2021-03-03T23:44:45Z

@rhc54 Ok. I can re-enable that. I added a sort priority order but we can tweak it:

config files (opal_config.h, etc)
in-tree files ("" includes)
system files (<> includes)

You can force sorting header with --sort-includes but if the consensus is that we should always keep them sorted then we should absolutely set SortIncludes to true.

hjelmn · 2021-03-03T23:47:02Z

Haven't figured out how yet but we might also want to disallow bare includes (includes without a full path). For example: allow "opal/mca/btl/btl/base/base.h" but disallow "base.h" from the files in that directory. Then we could also sort opal, then ompi, then oshmem headers (or some variation on that). Bare header includes make that difficult.

bwbarrett · 2021-03-04T01:28:52Z

It looks like you changed it, but Open MPI style was supposed to not have a space between function name and the open paren. So I think your initial commit message was wrong.

hjelmn · 2021-03-04T02:42:05Z

@bwbarrett True. Will fix the commit message on squish and will fix the PR description now.

jsquyres · 2021-03-09T16:52:56Z

Per discussion on the 9 March 2021, we'd like to get this in before we branch for v5.0.

Unless there are objections, let's do this:

Need to add exclusions for treematch and romio and the 3rd-party tree
Add a Github Action to check future PRs
Merge this PR
Make another PR to actually apply all these style fixes across our in-tree code (e.g., not romio/treematch/3rd-party)

awlauria · 2021-03-09T17:01:11Z

@gpaulsen and I are OK with adding this to the 5.0 post branch allow list if it needs extra time.

hjelmn · 2021-03-09T23:21:39Z

Added some details to the top of .clang-format, squished everything, and updated the commit message.

.clang-format

hjelmn · 2021-03-10T18:54:07Z

Any interest in this feature:

IndentPPDirectives: AfterHash

This style is used in a couple of places and does help readability. It turns:

#if FOO
#define BAR 1
#endif

into

#if FOO
#    define BAR 1
#endif

or we can use:

IndentPPDirectives: BeforeHash

which indents before the hash.

bosilca · 2021-03-10T18:59:22Z

How is this working with our [BEGIN | END]_C_DECLS ? If it increases the indentation for everything in the file (similar to emacs) then it is not good.

hjelmn · 2021-03-10T19:10:31Z

Let me check.

hjelmn · 2021-03-10T19:11:41Z

BEGIN_C_DECLS does not increase indentation because it doesn't start with a hash.

hjelmn · 2021-03-10T19:17:33Z

I just reformatted the whole code base as a test now to double check. BEGIN_C_DECLS definitely does not increase the indentation level. We can make that change if we ever wanted to by adding BEGIN_C_DECLS as a start block identifier and END_C_DECLS as an end block identifier. I know that is not what we want but putting it out there that we can add symbols that increase/decrease indentation.

awlauria · 2021-03-11T14:07:55Z

bot:ompi:retest

devreal · 2021-03-11T16:20:27Z

I applied the format to some files I've been working on recently and like most of what the current configuration currently produces. However, I noticed a few things that we might want to reconsider:

I prefer having no spaces after a cast operator, but that might just be my preference (although I think no-space is used mostly consistently at the moment)

-    index = (int)(uintptr_t)pvar->ctx;  /* Convert from MPI_T pvar index to SPC index */
+    index = (int) (uintptr_t) pvar->ctx; /* Convert from MPI_T pvar index to SPC index */

There is something with the & binary operator:

-    if ((assert & MPI_MODE_NOCHECK) == 0) {
+    if ((assert &MPI_MODE_NOCHECK) == 0) {

-> Why the change to &MPI_MODE_NOPRECEDE?

There are some instances where a deliberate line break inside an if condition helps readability, can we keep that?

-    if (module->epoch_type.access != NONE_EPOCH &&
-        module->epoch_type.access != FENCE_EPOCH) {
+    if (module->epoch_type.access != NONE_EPOCH && module->epoch_type.access != FENCE_EPOCH) {

It appears the formatting tries to maximize the line usage, which sometimes leads to weird formats, e.g.:

-                    ompi_osc_ucx_handle_incoming_post(module, &(module->state.post_state[j]), NULL, 0);
+                    ompi_osc_ucx_handle_incoming_post(module, &(module->state.post_state[j]), NULL,
+                                                      0);

I'd rather have the lines balanced:

-                    ompi_osc_ucx_handle_incoming_post(module, &(module->state.post_state[j]), NULL, 0);
+                    ompi_osc_ucx_handle_incoming_post(module, &(module->state.post_state[j]),
+                                                      NULL, 0);

That also applies to cases where lines are balanced at the moment and the formatter reshuffles them to maximize the first lines.

Can we enforce curly braces while reformatting?

-        if (OMPI_SUCCESS != ret) return ret;
+        if (OMPI_SUCCESS != ret)
+            return ret;

I'm not sure I like changes such as this one:

-        ret = opal_common_ucx_wpmem_fetch_nb(module->mem, opcode, value, target,
-                                             output_addr, origin_dt_bytes, remote_addr,
-                                             user_req_cb, user_req_ptr);
+        ret =
+            opal_common_ucx_wpmem_fetch_nb(module->mem, opcode, value, target, output_addr,
+                                           origin_dt_bytes, remote_addr, user_req_cb, user_req_ptr);

Is there a way to avoid the line break after the assignment?

Some of the block-aligned formatting may lead to strange formats, not sure this is desirable:

         module->epoch_type.exposure = NONE_EPOCH;
-        *flag = 1;
+        *flag                       = 1;

Maybe constrain this alignment to variable definition blocks?

hjelmn · 2021-03-11T17:28:57Z

@devreal Thanks for the feedback. Let me take a look now. The starting point was the LLVM format (default) tweaked for Open MPI. Likely are a couple of things we need to adjust.

bwbarrett · 2021-03-11T17:37:13Z

I had to go reread https://github.com/open-mpi/ompi/wiki/CodingStyle, because I swear we used to have a line about there being a space between cast and variable. It looks like I'm wrong, but it is certainly the common behavior in OMPI. I have no strong opinion, as long as we're consistent.

rhc54 · 2021-03-11T17:37:51Z

I prefer spaces after casts. Not sure about others. Either way is fine.

I usually don't put spaces after casts - but I don't have strong feelings over it.

bwbarrett · 2021-03-11T17:38:55Z

We definitely need to fix the curly braces missing around an added line break for a conditional. That's one of the only things in our coding style :).

hjelmn · 2021-03-11T17:45:29Z

Braces around single line conditionals can't be added by clang-format. Looks like that is something clang-tidy can do. It is a bit more heavyweight but it worth discussing separate from the clang-format discussion. I am used to having both but clang-format is the first step.

hjelmn · 2021-03-11T17:48:30Z

For the assignment alignment. The recommendation is if you want to break assignment alignment that there be a new line. I usually do that. We can also turn off aligning assignment if it is not desired.

Example:

  module.foo = bar;
  baz        = bar;

is how it will align it with AlignConsecutiveAssignments: true so you have to do:

  module.foo = bar;

  baz = bar;

to prevent the alignment. I can go either way. Sometimes I like assignment alignmed, sometimes I don't.

bwbarrett · 2021-03-11T17:50:30Z

I'd rather disable the assignment alignment check; we've never done that in the wide in OMPI.

devreal · 2021-03-11T17:53:25Z

Maybe do the same for the cast spacing? People have different tastes and it's not really critical. And reordering function arguments only to avoid overly long lines will allow people to pick an order that makes sense in the specific instance.

hjelmn · 2021-03-11T17:55:40Z

For cast spacing. Let me get a sense of how widly it is used. If more than half the code base doesn't use it I will remove it otherwise I will leave it.

hjelmn · 2021-03-11T18:09:43Z

And the result is....

SpaceAfterCStyleCast: true is more common. This is after reformatting the entire code base (minus contrib).

SpaceAfterCStyleCast: true:

Mac-mini:ompi hjelmn$ git diff | wc
  531996 2168381 24469774

SpaceAfterCStyleCast: false:

Mac-mini:ompi hjelmn$ git diff | wc
  532623 2160783 24508410

hjelmn · 2021-03-11T18:28:08Z

As for the bit-wise and spacing. Looks like it thinks that we are taking the address of MPI_MODE_NOCHECK. Looking at how to address that one.

hjelmn · 2021-03-11T18:33:32Z

Yup. That is something unique to the & bit-wise operator. Looks like a bug in clang-format as it should be able to tell the difference between taking the address and bit-wise & here.

hjelmn · 2021-03-11T18:46:04Z

Definitely a bug. Happens with '*' as well. It is deciding it is a unary not a binary operator. I found the relevant code in clang and will see if I can fix it. I already have one PR up on clang-format (https://reviews.llvm.org/D98429).

edit: Interesting. It only happens if the & happens inside parenthesis. This formats correctly: if (x & FOO) but this doesn't: if (!(x & FOO)). That is definitely a bug as this should be formatted as a binary operator with a single space on either side of the operator. Working on the fix now.

Ok. I see the problem. assert is treated differently. If you change the variable name from assert it works correctly. We really shouldn't be naming things like that!

hjelmn · 2021-03-11T19:30:55Z

Example:

$ clang-format --style=file --assume-filename=foo.c /tmp/foo.c
int main()
{
    int x = 16;
    if ((x & BAR) == 0) {
        return 1;
    }
    if ((assert &BAR) == 0) {
        return 2;
    }

    return 0;
}

So, lets fix by renaming all assert variables to something like mpi_assert.

hjelmn · 2021-03-11T22:24:33Z

Added a couple to tweaks that give slightly better formatting.

awlauria · 2021-03-16T15:39:50Z

@hjelmn any update on this? Can you squash down so we can get this in when you have a chance. thanks

This file works with clang-format --style=file to reformat code to match the style used in Open MPI. This type includes: - No tabs. They are not recommended for Open MPI and can often screw up the formatting. - Tab depth: 4. This is what is used throughout the Open MPI code base. - Max column width: 100. There currently is no standard for Open MPI but we should aim to use something reasonable. - Braces following function definitions occur un-indented on the following line. - Braces following other control statements occur on the same line as the control statement. - Spaces before open parentheses for control statements. No spaces otherwise. This is common accross the code base but not consistent. - Sort headers. Put _config headers before any other header. Followed by internal ("" headers), then system headers (<> headers). - Align consecutive macro definitions. Signed-off-by: Nathan Hjelm <[email protected]>

hjelmn · 2021-03-17T16:56:31Z

Squashed and good to go.

hjelmn · 2021-03-17T16:57:29Z

Just did a trial run on ompi and opal and it builds fine with one small exception (PR already up). Spot checked it and looks good. We can always tweak the format as we go.

awlauria · 2021-03-17T18:46:16Z

bot:aws:retest

awlauria · 2021-03-18T01:33:13Z

Thanks @hjelmn can you bring this back to 5.0.x?

This file works with clang-format --style=file to reformat code to match the style used in PMIx. This type includes: No tabs. They are not recommended for PMIx and can often screw up the formatting. Tab depth: 4. This is what is used throughout the PMIx code base. Max column width: 100. There currently is no standard for PMIx but we should aim to use something reasonable. Braces following function definitions occur un-indented on the following line. Braces following other control statements occur on the same line as the control statement. Spaces always before open parentheses for control statements (if, while, do, etc). This is common across the code base but not consistent. No spaces after function names, and function or macro calls. Align consecutive macro definitions. Tracks open-mpi/ompi#8551 Signed-off-by: Ralph Castain <[email protected]>

This file works with clang-format --style=file to reformat code to match the style used in PRRTE. This type includes: No tabs. They are not recommended for PRRTE and can often screw up the formatting. Tab depth: 4. This is what is used throughout the PRRTE code base. Max column width: 100. There currently is no standard for PRRTE but we should aim to use something reasonable. Braces following function definitions occur un-indented on the following line. Braces following other control statements occur on the same line as the control statement. Spaces always before open parentheses for control statements (if, while, do, etc). This is common across the code base but not consistent. No spaces after function names, and function or macro calls. Align consecutive macro definitions. Tracks open-mpi/ompi#8551 Signed-off-by: Ralph Castain <[email protected]>

This file works with clang-format --style=file to reformat code to match the style used in PMIx. This type includes: No tabs. They are not recommended for PMIx and can often screw up the formatting. Tab depth: 4. This is what is used throughout the PMIx code base. Max column width: 100. There currently is no standard for PMIx but we should aim to use something reasonable. Braces following function definitions occur un-indented on the following line. Braces following other control statements occur on the same line as the control statement. Spaces always before open parentheses for control statements (if, while, do, etc). This is common across the code base but not consistent. No spaces after function names, and function or macro calls. Align consecutive macro definitions. Tracks open-mpi/ompi#8551 Signed-off-by: Ralph Castain <[email protected]> (cherry picked from commit 57a3969)

This file works with clang-format --style=file to reformat code to match the style used in PMIx. This type includes: No tabs. They are not recommended for PMIx and can often screw up the formatting. Tab depth: 4. This is what is used throughout the PMIx code base. Max column width: 100. There currently is no standard for PMIx but we should aim to use something reasonable. Braces following function definitions occur un-indented on the following line. Braces following other control statements occur on the same line as the control statement. Spaces always before open parentheses for control statements (if, while, do, etc). This is common across the code base but not consistent. No spaces after function names, and function or macro calls. Align consecutive macro definitions. Tracks open-mpi/ompi#8551 Signed-off-by: Ralph Castain <[email protected]> (cherry picked from commit 57a3969) (cherry picked from commit 467f88e)

hjelmn added enhancement Target: main labels Mar 3, 2021

hjelmn requested review from jsquyres, bosilca and bwbarrett March 3, 2021 20:20

hjelmn requested a review from hppritcha March 3, 2021 20:21

hjelmn force-pushed the add_a_clang_format_file_that_can_be_used_to_reformat_code_to_the_recommended_ompi_stle branch from 2bc5868 to 0d35f5c Compare March 3, 2021 20:28

bosilca approved these changes Mar 3, 2021

View reviewed changes

hjelmn force-pushed the add_a_clang_format_file_that_can_be_used_to_reformat_code_to_the_recommended_ompi_stle branch from 9b213bc to 858e80e Compare March 9, 2021 23:20

rhc54 reviewed Mar 9, 2021

View reviewed changes

.clang-format Outdated Show resolved Hide resolved

rhc54 reviewed Mar 9, 2021

View reviewed changes

.clang-format Outdated Show resolved Hide resolved

hjelmn force-pushed the add_a_clang_format_file_that_can_be_used_to_reformat_code_to_the_recommended_ompi_stle branch from f2457df to 84640d6 Compare March 11, 2021 22:25

hjelmn force-pushed the add_a_clang_format_file_that_can_be_used_to_reformat_code_to_the_recommended_ompi_stle branch from 84640d6 to c352fba Compare March 17, 2021 15:59

awlauria merged commit 09eb050 into open-mpi:master Mar 17, 2021

rhc54 mentioned this pull request Mar 18, 2021

Add clang format support openpmix/openpmix#2106

Merged

rhc54 mentioned this pull request Mar 18, 2021

Add clang format support openpmix/prrte#844

Merged

gpaulsen mentioned this pull request Mar 19, 2021

clang-format: tweaks #8655

Merged

Add a .clang_format file for Open MPI #8551

Add a .clang_format file for Open MPI #8551

Uh oh!

Conversation

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

Uh oh!

Uh oh!

hjelmn commented Mar 3, 2021

Uh oh!

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

Uh oh!

Uh oh!

bosilca commented Mar 3, 2021

Uh oh!

hjelmn commented Mar 3, 2021

Uh oh!

hjelmn commented Mar 3, 2021

Uh oh!

hjelmn commented Mar 3, 2021

Uh oh!

bwbarrett commented Mar 4, 2021

Uh oh!

hjelmn commented Mar 4, 2021

Uh oh!

jsquyres commented Mar 9, 2021

Uh oh!

awlauria commented Mar 9, 2021

Uh oh!

hjelmn commented Mar 9, 2021

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

bosilca commented Mar 10, 2021

Uh oh!

hjelmn commented Mar 10, 2021

Uh oh!

hjelmn commented Mar 10, 2021

Uh oh!

hjelmn commented Mar 10, 2021

Uh oh!

awlauria commented Mar 11, 2021

Uh oh!

devreal commented Mar 11, 2021

Uh oh!

hjelmn commented Mar 11, 2021

Uh oh!

bwbarrett commented Mar 11, 2021

Uh oh!

rhc54 commented Mar 11, 2021

Uh oh!

bwbarrett commented Mar 11, 2021

Uh oh!

hjelmn commented Mar 11, 2021

Uh oh!

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

Uh oh!

Uh oh!

bwbarrett commented Mar 11, 2021

Uh oh!

devreal commented Mar 11, 2021

Uh oh!

hjelmn commented Mar 11, 2021

Uh oh!

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

Uh oh!

Uh oh!

hjelmn commented Mar 11, 2021

Uh oh!

hjelmn commented Mar 11, 2021

Uh oh!

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

Uh oh!

Uh oh!

hjelmn commented Mar 11, 2021

Uh oh!

hjelmn commented Mar 11, 2021

Uh oh!

hjelmn commented Mar 3, 2021 •

edited

Loading

hjelmn commented Mar 3, 2021 •

edited

Loading

hjelmn commented Mar 10, 2021 •

edited

Loading

hjelmn commented Mar 11, 2021 •

edited

Loading

hjelmn commented Mar 11, 2021 •

edited

Loading

hjelmn commented Mar 11, 2021 •

edited

Loading