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 file #123510

Closed
workingjubilee opened this issue Apr 5, 2024 · 8 comments · Fixed by #123918
Closed

Add .clang-format file #123510

workingjubilee opened this issue Apr 5, 2024 · 8 comments · Fixed by #123918
Assignees
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-llvm Working group: LLVM backend code generation

Comments

@workingjubilee
Copy link
Member

workingjubilee commented Apr 5, 2024

Is it possible to add a .clang-format file to configure the whole llvm-wrapper? The exists code does not fit any preset styles of clang-format. I have asked at discord but get no response about what code convention the cpp code follows.

Originally posted by @Lambdaris in #123437 (comment)

@Zalathar mentioned we have no such automated infra in place. We have many open issues for removing this C++ code by effectively upstreaming our wrappers to LLVM, but I agree, we could start by enforcing a style.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 5, 2024
@workingjubilee workingjubilee added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-FFI Area: Foreign function interface (FFI) WG-llvm Working group: LLVM backend code generation A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-FFI Area: Foreign function interface (FFI) labels Apr 5, 2024
@DianQK
Copy link
Member

DianQK commented Apr 7, 2024

I would like to add a check that follows to the LLVM style and run this check on CI.

I'm uncertain about adding a check locally, since most people won't be modifying the C++ code. It would require users to install clang-format on their local machines. Perhaps we could skip the check locally or only run it if clang-format is detected in the user's $PATH.

@rustbot claim

@Zalathar
Copy link
Contributor

Zalathar commented Apr 7, 2024

I wonder if it would be worthwhile to build clang-format as part of our LLVM build, and use that.

Obviously that doesn't help in cases where LLVM is downloaded (unless we also include it in the download), but maybe it's fine to skip the local check in those cases.

@workingjubilee
Copy link
Member Author

As long as our C++ code doesn't introduce additional barriers to contributing to the Rust code.

@DianQK
Copy link
Member

DianQK commented Apr 8, 2024

Currently, there are no clang-related builds here. Typically, we don't need to ensure that clang-format uses the same version of the code. I think that a local warning would be sufficient.

@durin42
Copy link
Contributor

durin42 commented Apr 10, 2024

For Mercurial we found we had to be pretty particular about what version of clang-format we used in our test coverage, or some things would be fussy in some corner cases. I imagine that would be the case for us too, and it makes sense to me to take clang-format from the LLVM build so it's a consistent version...

@DianQK
Copy link
Member

DianQK commented Apr 11, 2024

If the cost is low, I would opt for a consistent version. In upstream LLVM practices, using version 16 of clang-format is also compatible with developing LLVM 18/19. Since we have very little C++ code, I don't think it's worth distributing clang-format. :)

Of course, to ensure consistent formatting results, I will add the corresponding checks to the CI. I think we could initially implement a non-blocking CI check for a while to see if we encounter any inconsistencies.

@DianQK
Copy link
Member

DianQK commented Jun 9, 2024

After some attempts, I agreed to add clang-format to our LLVM build, primarily because I found differences between clang-format-15 and clang-format-18. Additionally, it's challenging to have both Python 2.7 and clang-format-18 on the CI simultaneously. Fortunately, adding clang-format is much easier than adding clang. We don't need to worry about the various header file lookup issues. I will prepare an MCP for this change.

@workingjubilee
Copy link
Member Author

cool, thanks for implementing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-llvm Working group: LLVM backend code generation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants