Skip to content
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.

Sign up for GitHub

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

Jump to bottom

1699: Update .clang-format syntax #1700

Merged
merged 4 commits into from
Mar 21, 2022
Merged

1699: Update .clang-format syntax #1700

merged 4 commits into from
Mar 21, 2022

Conversation

cz4rs
Copy link
Contributor

@cz4rs cz4rs commented Mar 14, 2022 •
edited
Loading

  • Update .clang-format to match Clang 13 syntax.
  • Add post-commit hook to format modified code
    • requires git clang-format plugin
    • post-commit works better for me, but this could be easily adapted to pre-commit as well

fixes #1699

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (gcc-5, ubuntu, mpich)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (gcc-6, ubuntu, mpich)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (gcc-9, ubuntu, mpich, zoltan)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (clang-5.0, ubuntu, mpich)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (clang-3.9, ubuntu, mpich)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (gcc-10, ubuntu, openmpi, no LB)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (gcc-8, ubuntu, mpich, address sanitizer)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (clang-9, ubuntu, mpich)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (nvidia cuda 10.1, ubuntu, mpich)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (gcc-7, ubuntu, mpich, trace runtime, LB)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (clang-10, alpine, mpich)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (nvidia cuda 11.0, ubuntu, mpich)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 14, 2022 •
edited
Loading

PR tests (clang-10, ubuntu, mpich)

Build for 83f4435

Compilation - successful

Testing - passed

Build log

@cz4rs cz4rs marked this pull request as ready for review March 14, 2022 11:42
jstrzebonski
jstrzebonski reviewed Mar 14, 2022
View reviewed changes
scripts/post-commit Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 14, 2022 •
edited
Loading

Codecov Report

Merging #1700 (83f4435) into develop (2a6319a) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1700   +/-   ##
========================================
  Coverage    83.71%   83.72%           
========================================
  Files          772      772           
  Lines        26797    26797           
========================================
+ Hits         22433    22435    +2     
+ Misses        4364     4362    -2
Impacted Files Coverage Δ
src/vt/messaging/request_holder.h 76.92% <0.00%> (+7.69%) ⬆️

@cz4rs cz4rs force-pushed the 1699-update-clang-format branch from 98d61c7 to dfa0f02 Compare March 14, 2022 11:53
@cz4rs cz4rs requested a review from jstrzebonski March 14, 2022 11:55
jstrzebonski
jstrzebonski approved these changes Mar 14, 2022
View reviewed changes
Copy link
Contributor

@jstrzebonski jstrzebonski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 Checked with my clang-format 12.
As for the script, some folks might find it handy, thanks.

nmm0
nmm0 reviewed Mar 15, 2022
View reviewed changes
Copy link
Collaborator

@nmm0 nmm0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to also change the following values:

AlignConsecutiveAssignments: None
AlignConsecutiveDeclarations: None
AlignConsecutiveMacros: None
SortIncludes: Never

@cz4rs
Copy link
Contributor Author

cz4rs commented Mar 15, 2022 •
edited
Loading

I had to also change the following values:

AlignConsecutiveAssignments: None
AlignConsecutiveDeclarations: None
AlignConsecutiveMacros: None
SortIncludes: Never

I will update those and check with Clang 13 👍

edit: I have Clang 10 at hand as well, this should be reasonable threshold for backwards compatibility.

@lifflander lifflander mentioned this pull request Mar 15, 2022
Open
@PhilMiller PhilMiller force-pushed the 1699-update-clang-format branch from dfa0f02 to c09d229 Compare March 16, 2022 05:30
@cz4rs cz4rs force-pushed the 1699-update-clang-format branch from c09d229 to db0ce96 Compare March 16, 2022 15:19
@cz4rs cz4rs requested a review from nmm0 March 16, 2022 15:21
@cz4rs
Copy link
Contributor Author

cz4rs commented Mar 16, 2022 •
edited
Loading

@nmm0 Updated the remaining values. Works fine for me, but older version (clang-format-10) didn't like it.

If anyone with an older setup complains we can discuss the solution. For now, I think targeting newer version is the right choice.

@jstrzebonski
Copy link
Contributor

My clang-format (12.0.0) doesn't really like SortIncludes: Never ;- ) but I can live with that.

lifflander
lifflander approved these changes Mar 16, 2022
View reviewed changes
Copy link
Collaborator

@lifflander lifflander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this post-commit hook run automatically or how does it work?

@cz4rs
Copy link
Contributor Author

cz4rs commented Mar 16, 2022 •
edited
Loading

Does this post-commit hook run automatically or how does it work?

You need to copy it to .git/hooks/.

@cz4rs cz4rs force-pushed the 1699-update-clang-format branch from d344f35 to 5903f33 Compare March 21, 2022 11:50
@cz4rs cz4rs force-pushed the 1699-update-clang-format branch from 5903f33 to 83f4435 Compare March 21, 2022 16:46
@nlslatt nlslatt merged commit c4e4385 into develop Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jstrzebonski jstrzebonski jstrzebonski approved these changes

@JacobDomagala JacobDomagala JacobDomagala approved these changes

@lifflander lifflander lifflander approved these changes

@nlslatt nlslatt Awaiting requested review from nlslatt

@PhilMiller PhilMiller Awaiting requested review from PhilMiller

@nmm0 nmm0 Awaiting requested review from nmm0

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

migrate .clang-format to Clang 13 syntax
6 participants
@cz4rs @jstrzebonski @lifflander @nmm0 @JacobDomagala @nlslatt