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

Add clang-format and run it #310

Closed
wants to merge 3 commits into from
Closed

Add clang-format and run it #310

wants to merge 3 commits into from

Conversation

erlingrj
Copy link
Collaborator

In this PR I copy over the formatter setup from reactor-cpp. I think we should use a formatter, we can argue about what the settings of it should be. Currently I put it at a column width of 80. Reactor-cpp uses 120.

The bigger question is when we should apply it. It will create problems for any outstanding feature-branch...

@erlingrj erlingrj requested review from lhstrh, edwardalee and petervdonovan and removed request for lhstrh November 23, 2023 14:01
@erlingrj
Copy link
Collaborator Author

It is not urgent to make a decision, I think ideally we make a plan for when to apply it. Some point in the future when we have merged most of the big outstanding work.

@edwardalee
Copy link
Contributor

There are several outstanding PRs, so this does not seem like a good time to do this. But it is something we should do.

@cmnrd
Copy link
Contributor

cmnrd commented Feb 15, 2024

There is never a good time for something like this. If we want to do it (which I think we should), then we have to bite the bullet (like we did in the main LF repo).

@edwardalee
Copy link
Contributor

Hmm... doing this now will likely result in my ongoing cleanups getting abandoned... can we coordinate and it between major phases of this? I'm in the middle of one that has hundreds, maybe thousands of line changed. @erlingrj also has some large pending work.

@cmnrd
Copy link
Contributor

cmnrd commented Feb 15, 2024

Sure, I just noticed this PR today and wanted to ping it and point out that there is never a perfect time. But if there is a concrete milestone to wait for, that is fine.

@edwardalee
Copy link
Contributor

How about instead providing the docs on how to set VS Code to do this formatting (in the developer section of the handbook), and also the instructions for how to do the mass formatting, and I will coordinate with @erling to find a good time to do this.

@cmnrd
Copy link
Contributor

cmnrd commented Feb 21, 2024

We currently don't have developer instructions for the individual runtimes in the handbook. And I think we should keep those instructions in the README of the runtimes. Independent of where we place it, here is a simple description:

We use clang-format for automatically formatting the source code. Please run clang-format <file-name> before you commit a change. To format all files in the repository, you can use the following command:

clang-format $(find -name "*.h" -o -name "*.c") 

Most IDEs provide support for formatting with clang-format. VS Code, for instance, has built-in support and automatically invokes clang-format when you use the IDE's formatting functionality.

@erlingrj
Copy link
Collaborator Author

erlingrj commented Mar 5, 2024

See #384 for clang-format run on main

@erlingrj erlingrj closed this Mar 5, 2024
@erlingrj erlingrj deleted the use-formatter branch May 6, 2024 09:12
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