Skip to content

opal: run clang-format on all of opal #8647

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

Merged
merged 2 commits into from
Mar 19, 2021

Conversation

hjelmn
Copy link
Member

@hjelmn hjelmn commented Mar 18, 2021

Some edits had to be done by hand as opal_datatype_internal.h is incorrectly
detected as objective-c. This happens because the OPAL_DATATYPE_INIT_PTYPES_ARRAY
macro seems to trip up clang-format. I intend to fix this bug upstream but
for now it was fine to delete this definition, format the code, then re-add
it to match the style guidelines.

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

@hjelmn
Copy link
Member Author

hjelmn commented Mar 18, 2021

If it makes sense this can be broken down into many commits. Not sure what is preferred.

@hjelmn
Copy link
Member Author

hjelmn commented Mar 18, 2021

:bot:retest:aws

@bosilca
Copy link
Member

bosilca commented Mar 18, 2021

Let us suffer only once please.

@hjelmn
Copy link
Member Author

hjelmn commented Mar 18, 2021

Ok. Let me run clang-tidy on opal to add braces to single-line control statements and add it as another commit here so that we get all the opal hurt at once. Will be adding the script to run it to contrib so it can be run whenever someone want to use it.

@hjelmn hjelmn force-pushed the master_format_opal branch from aba87c4 to 4f50cc4 Compare March 19, 2021 16:32
hjelmn added 2 commits March 19, 2021 10:43
Some edits had to be done by hand as opal_datatype_internal.h is incorrectly
detected as objective-c. This happens because the OPAL_DATATYPE_INIT_PTYPES_ARRAY
macro seems to trip up clang-format. I intend to fix this bug upstream but
for now it was fine to delete this definition, format the code, then re-add
it to match the style guidelines.

Signed-off-by: Nathan Hjelm <[email protected]>
To match Open MPI style guidelines all control statements must use braces
even if they have a single statement. This commit fixes all sources that
compile on macOS (limitation of clang-tidy that sources need to be
compilable) in the opal subdir to match this guideline.

Signed-off-by: Nathan Hjelm <[email protected]>
@hjelmn hjelmn force-pushed the master_format_opal branch from 4f50cc4 to 5c6c7ff Compare March 19, 2021 16:43
@hjelmn
Copy link
Member Author

hjelmn commented Mar 19, 2021

Ok, this should be good now. I re-ran clang-format with the setting update then ran clang-tidy to add the missing braces and out that in as a separate commit.

@hjelmn hjelmn merged commit 26c136a into open-mpi:master Mar 19, 2021
@gpaulsen
Copy link
Member

@hjelmn Can you please PR this along with other formatting PRs on master to v5.0.x?

Some other PRs that we need now may include:
#8639, (this #8647), #8668, #8674, #8676.

@gpaulsen
Copy link
Member

@hjelmn I'm getting reports that it's hard to cherry-pick opal changes to v5.0.x, presumably because we're missing the above.
I don't have clang-format v10/11 on our systems yet. Would you mind cherrypicking (or rerunning) this (and possibly the others mentioned above) to v5.0.x please?

@awlauria
Copy link
Contributor

We should probably merge as many pr's (preferably all of them) from master to v5 first that are coming. That would probably help reduce the pain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants