Skip to content

Conversation

@st0012
Copy link
Member

@st0012 st0012 commented Apr 30, 2025

While working on #2398, we found it necessary to have some sort of auto-formatting for C code as well to ensure consistent coding style. But instead of adding it as part of #2398, which already introduces many changes on the implementation itself, I think it'd be better to have the auto-formatting setup in a separate PR.

This PR:

  • Sets up clang-format for the project, including
    • .clang-format defines the rules of formatting (like rubocop.yml)
    • A new format:c rake task will perform the formatting to all C code under ext/, src/, and include/
    • A new format:c_check rake task will check if formatting is required without actually making the changes
    • A new CI workflow to perform format:c_check
    • Introductions about C formatting in the readme
  • Sets up auto-formatting in VS Code, including:
    • Extension recommendation for the formatting
    • Workspace settings to enable it
  • Formats the code with the given rules. The most noticeable changes are indentation related. Currently we have both 2-space and 4-space based indentation. Since the majority of the code uses 2-space indentation, which is what Introduce standalone C parser for RBS with arena allocation #2398 mostly follows, I set the rule to 2 and update the code that current indents with 4 spaces

@st0012 st0012 force-pushed the setup-clang-format branch 2 times, most recently from 1433d21 to 92f5cbc Compare April 30, 2025 05:34
@st0012 st0012 marked this pull request as draft April 30, 2025 05:48
@st0012 st0012 force-pushed the setup-clang-format branch 6 times, most recently from 58585d3 to 44781eb Compare April 30, 2025 13:10
@st0012 st0012 force-pushed the setup-clang-format branch from 44781eb to 334cd97 Compare April 30, 2025 13:20
@st0012 st0012 force-pushed the setup-clang-format branch 2 times, most recently from ac540b3 to f06867e Compare April 30, 2025 14:00
@st0012 st0012 marked this pull request as ready for review April 30, 2025 14:01
@st0012 st0012 force-pushed the setup-clang-format branch from f06867e to 29251b7 Compare May 1, 2025 08:24
@soutaro
Copy link
Member

soutaro commented May 2, 2025

I'm good for clang-format. Can you add a commit for 4-space indentation?

@soutaro soutaro added this to the RBS 4.0 milestone May 2, 2025
@st0012 st0012 force-pushed the setup-clang-format branch from 29251b7 to d0abc94 Compare May 4, 2025 03:11
@st0012 st0012 requested a review from soutaro May 4, 2025 15:14
@st0012
Copy link
Member Author

st0012 commented May 4, 2025

Updated the indent width to 4 👍

@st0012 st0012 force-pushed the setup-clang-format branch from 0543ef3 to 13f9dc3 Compare May 12, 2025 08:56
@st0012 st0012 force-pushed the setup-clang-format branch 2 times, most recently from 2cd788d to 8a7cb1b Compare May 12, 2025 09:35
@st0012 st0012 force-pushed the setup-clang-format branch from 8a7cb1b to 013584a Compare May 12, 2025 09:43
run: |
sudo apt-get update
sudo apt-get install -y libdb-dev curl autoconf automake m4 libtool python3
- name: Install Re2c
Copy link
Member Author

Choose a reason for hiding this comment

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

@soutaro I decided to move all checking related tasks to the new c-check workflow so we can simplify the ruby builds too. WDYT?

@@ -0,0 +1,74 @@
---
Language: Cpp
Copy link
Member Author

Choose a reason for hiding this comment

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

We have to explicitly list many rules here to reduce/avoid differences between clang-format versions, which is hard to enforce across dev machines and CI.

@st0012
Copy link
Member Author

st0012 commented May 12, 2025

@soutaro I think it's ready for review

Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

Thanks!

@soutaro
Copy link
Member

soutaro commented May 14, 2025

Updated the branch protection setting to require format-check job.

@soutaro soutaro added this pull request to the merge queue May 14, 2025
github-merge-queue bot pushed a commit that referenced this pull request May 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 14, 2025
@soutaro soutaro added this pull request to the merge queue May 15, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 15, 2025
@soutaro soutaro added this pull request to the merge queue May 15, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 15, 2025
@soutaro soutaro added this pull request to the merge queue May 15, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 15, 2025
bundler-cache: none
- name: Set working directory as safe
run: git config --global --add safe.directory $(pwd)
- name: Set up permission
Copy link
Member

Choose a reason for hiding this comment

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

This step causes an error...
(Other jobs are fixed in #2485.)

@soutaro soutaro merged commit ba65078 into ruby:master May 15, 2025
22 checks passed
@soutaro
Copy link
Member

soutaro commented May 15, 2025

Merged this PR to continue working on other PRs.

@soutaro soutaro mentioned this pull request May 15, 2025
@st0012 st0012 deleted the setup-clang-format branch May 15, 2025 11:00
@soutaro soutaro mentioned this pull request Oct 6, 2025
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.

4 participants