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.

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

maint: Update clang-format to v19 #3600

Merged
merged 4 commits into from
Nov 18, 2024

Conversation

mathbunnyru
Copy link
Contributor

@mathbunnyru mathbunnyru commented Nov 12, 2024

Possible options (I didn't change any): https://clang.llvm.org/docs/ClangFormatStyleOptions.html

@jjerphan jjerphan added release::ci_docs For PRs related to CI or documentation release::bug_fixes For PRs fixing bugs and removed release::ci_docs For PRs related to CI or documentation labels Nov 18, 2024
@jjerphan jjerphan changed the title Update clang-format to v19 maint: Update clang-format to v19 Nov 18, 2024
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM if and only if the merge commit of this PR has its diff ignored since it bring no semantic changes.

For more information, see:

@Hind-M
Copy link
Member

Hind-M commented Nov 18, 2024

To have a consistent history and since the formatting commits were always explicit, I think we should actually just leave the files diff as is (not ignored) so that we know that a change was made regarding the format.

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

OK, such diffs can actually be ignored locally by developers if they want to.

@Hind-M
Copy link
Member

Hind-M commented Nov 18, 2024

@Klaim Any objections on the format result? (We may change the options if any).

@Klaim
Copy link
Member

Klaim commented Nov 18, 2024

@Klaim Any objections on the format result? (We may change the options if any).

Looks tolerable to me. 👍🏽

To be completely clear, there are things I'm not a fan of but that I can totally live with:

  • having the return type on a second line when using the auto ... -> function signature syntax is nice, but I would prefer the short signatures to not have that second line, as it seems not necessary (only on short signatures) - but I can live with systematic return type separated;
  • My preferred style of lambda expression is to have the { on the same line as the signature (like some post-java languages?), so that the content of the body looks like it's part of the expression hosting the lambda expression (func(a, b, []{ ...), and it avoids having too many lines in short lambdas, and also that makes the lambda expressions standout stylistically when you use a different style for most C++ expressions ({ and } always on a new line for example, to mark a scope clearly); here we get a new line per { systematically and the signature is kind of disconnected visually - but it's really minor and I can also totally live with it;
  • (not modified here) I dont like to be forced to add explicit scope for every if, because of the sometime unnecessary verbosity (wicth hurts maintenance) but I understand that it helps with debugging (which helps maintenance) so I can live with it too;

Also I don't know the limitations of clang-format regarding these minor points, maybe it's just not possible to have the exact style I prefer. Probably not worth our time if everybody can live with this style.

So I think we're good to go. 👍🏽

@jjerphan
Copy link
Member

I let anyone of you merge this PR.

@Hind-M Hind-M merged commit ad9b2d6 into mamba-org:main Nov 18, 2024
32 checks passed
@Hind-M Hind-M added release::ci_docs For PRs related to CI or documentation and removed release::bug_fixes For PRs fixing bugs labels Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release::ci_docs For PRs related to CI or documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants