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

clang-format 18 #1113

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

clang-format 18 #1113

wants to merge 20 commits into from

Conversation

graebm
Copy link
Contributor

@graebm graebm commented May 10, 2024

Issue:
We used clang-format 9 since forever, but it's not easy for people to get such an old version anymore.

Description of Changes:

  • Use clang-format 18 (latest), instead of 9
  • Use jidicula/clang-format-action Github Action in CI, instead of DoozyX/clang-format-lint-action which doesn't support 18 yet
  • Add -i option to format-check script, to edit files inplace
    • Also, rewrite script in python, so it can run on Windows, and because our team isn't great at shell scripts
  • Annoyingly, there are some differences between clang-format 18.1.3 (used by jidicula/clang-format-action and by the VSCode C/C++ extension) and clang-format 18.1.5 (default in Homebrew on Mac). I turned off formatting for those bits where they disagree.

Updating your Machine:

  • MacOS:
    • if you have clang-format 9 from homebrew, uninstall it: brew uninstall llvm@9
    • install 18: brew install clang-format
  • Any other machine:
    • uninstall any pre-existing clang-format versions
    • Just use VSCode's C/C++ extension to format, which uses clang-format 18 by default

Research:
We want to update to the newest version that can easily be installed on the platforms/IDEs we develop on:

  • macOS:8, 11, 18 (default) via homebrew
  • Windows: Not sure how to install it, but 18 is available via VSCode extension
  • Amazon Linux 2: 11 only via yum install clang-tools-extra
  • Amazon Linux 2023: 15 only via dnf install clang-tools-extra
  • Ubuntu 24: 18 via apt install clang-format
  • VSCode: 18 is bundled into the C/C++ extension, and it's what gets used if you don't have clang-format installed
    • Located here: ~/.vscode/extensions/ms-vscode.cpptools-1.20.5-darwin-x64/LLVM/bin/clang-format.

Since 18 is latest, and easily available on all platforms via the VSCode extension, let's use that.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.12%. Comparing base (24ac711) to head (32d5212).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1113   +/-   ##
=======================================
  Coverage   83.12%   83.12%           
=======================================
  Files          56       56           
  Lines        5754     5754           
=======================================
  Hits         4783     4783           
  Misses        971      971           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +39 to +41
/* clang-format off */
/* reenable formatting once we're all using 18.1.5+ */

Copy link
Contributor

Choose a reason for hiding this comment

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

Having to do this across 20 different repos and any new code we write is going to be annoying. Can we look into getting 18.1.5 in CI? Some options that I can think of

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting: You can use pipx to run clang-format, as well. For example, pipx run clang-format <args> will run clang-format without any previous install required on any machine with pipx (including all default GitHub Actions / Azure runners, avoiding requiring a pre-install step or even actions/setup-python).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already fixed up all the other repos on my machine. There's just one code pattern that's causing all these differences between 18.1.3 and 18.1.5. It's when we use a macro to init the fields of a structure. aws-c-common has way more instances of this than any other repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into the pip thing...

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.

None yet

3 participants