Skip to content

Clang format - #473

Open
lucbv wants to merge 5 commits into
kokkos:stablefrom
lucbv:clang_format
Open

lucbv wants to merge 5 commits into
kokkos:stablefrom
lucbv:clang_format

Conversation

@lucbv

@lucbv lucbv commented Sep 28, 2026

Copy link
Copy Markdown

No description provided.

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>
Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

@dalg24 dalg24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like we (I) messed up in #468 and that the pre-commit check is not running

on:
workflow_call:

Please change it to on: [push, pull_request]

@lucbv

lucbv commented Sep 28, 2026

Copy link
Copy Markdown
Author

Yes, I am actually inspecting why that is... the good news is that you are doing the some in kokkos/kokkos so there surely is a reason?
https://github.com/kokkos/kokkos/blob/develop/.github/workflows/pre-commit.yml

@masterleinad

Copy link
Copy Markdown
Contributor

This is essentially the same as #414.

@dalg24

dalg24 commented Sep 28, 2026

Copy link
Copy Markdown
Member

@lucbv

lucbv commented Sep 28, 2026 •

Copy link
Copy Markdown
Author

@masterleinad that's a good point, although since pre-commit is being used for the license already maybe that makes this approach more appealing?
Also is there a will to apply all code changes in one large commit or should it be done incrementally when people touch various files of the library in PRs?

@dalg24 thanks for pointing out how this gets handled in kokkos, it makes sense there but probably not as much here so I will modify the workflow

@masterleinad

Copy link
Copy Markdown
Contributor

Yes, I am actually inspecting why that is... the good news is that you are doing the some in kokkos/kokkos so there surely is a reason?

We are actually using workflow dispatch in Kokkos Core.

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>
@masterleinad

Copy link
Copy Markdown
Contributor

@masterleinad that's a good point, although since pre-commit is being used for the license already maybe that makes this approach more appealing?

I just wanted to make sure you have seen that one including its reviews and avoid duplicating efforts.

@lucbv

lucbv commented Sep 28, 2026

Copy link
Copy Markdown
Author

I just wanted to make sure you have seen that one including its reviews and avoid duplicating efforts.

Thanks for pointing it out, I was not aware when I made this PR. At least this revealed the lack of running check for license detection. Also I mostly did it so I can test this before doing it in Kokkos Kernels, so I'm really using mdspan as a guinea-pig 🫢

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>
Some how one formatting diff got away!

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>
@lucbv
lucbv requested a review from dalg24 September 29, 2026 15:16
Comment on lines -3 to -4
on:
workflow_call:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure if this should be in a separate pull request.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed, here is my rational:

  • it is required for the clang-format check to work
  • the license check passes so it did not generate additional changes unrelated to formatting

If we needed changes because of the license check I would lean for two PRs but here I can get away with one in my opinion

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just added another pull request just for this change, #475.

Comment thread .clang-format

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@crtrott @nmm0 please advise on what format you would like to use.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants