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

Format with clang-foramt 18.1 #840

Merged
merged 2 commits into from
Mar 27, 2024
Merged

Format with clang-foramt 18.1 #840

merged 2 commits into from
Mar 27, 2024

Conversation

Bambooin
Copy link
Contributor

@Bambooin Bambooin commented Mar 7, 2024

Pull request

Issue tracker

Fixes will automatically close the related issue

Fixes #

Feature

Describe feature of pull request

Format code with clang-format 18.1.

It's hard for Homebrew to pin or install the older version formula.

Maybe we can use other solution to install specific clang-format version?

Unit test

  • Done

Manual test

  • Done

Code Review

  1. Unit and manual test pass
  2. GitHub Action CI pass
  3. At least one contributor reviews and votes
  4. Can be merged clean without conflicts
  5. PR will be merged by rebase upstream base

Additional Info

Copy link
Member

@lotem lotem left a comment

Choose a reason for hiding this comment

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

I'm concerned that the result in CI can be different from running make clang-format-apply locally.

@Bambooin
Copy link
Contributor Author

Yes, you are right, I have this issue in my local macOS with clang-format 18.

In local homebrew, the clang-format is 18.1.2 right now.

$ brew info clang-format
==> clang-format: stable 18.1.2 (bottled), HEAD

Due to homebrew is hard to install old version: https://github.com/orgs/Homebrew/discussions/155

I wondered that we should install and pin with clang-format 17 in https://apt.llvm.org/

But this will make developers on macOS unhapppy, if we always follow the latest clang-format, it will make developers on other platforms unhappy too.

Any better idea are welcome.

Copy link
Member

@lotem lotem left a comment

Choose a reason for hiding this comment

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

There is no perfect way to smoothly upgrade clang-format for all people.
Let's do it now.

@lotem lotem merged commit bbdc3e1 into rime:master Mar 27, 2024
10 checks passed
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.

2 participants