Skip to content

Add .clang-format and run clang-format -i *.cpp #84889

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

Closed
wants to merge 1 commit into from

Conversation

est31
Copy link
Member

@est31 est31 commented May 4, 2021

It seems to me the original intent of the code style of the
LLVM wrapper was to follow the LLVM format. However, code style has
gotten a bit inconsistent inside the files. Thus, we add a
.clang-format file that indicates that the LLVM format is followed,
and run clang-format -i *.cpp on the files.

I file this PR as a request-for-comment. Please suggest some other code style if you prefer it. clang-format has a rich set of configuration options.

Also I wonder if CI should enforce this. It's easy to do, one just has to run clang-format --dry-run --Werror *.cpp in the llvm-wrapper directory. Should it maybe be added to tidy?

It seems to me the original intent of the code style of the
LLVM wrapper was to follow the LLVM format. However, code style has
gotten a bit inconsistent inside the files. Thus, we add a
.clang-format file that indicates that the LLVM format is followed,
and run clang-format -i *.cpp on the files.
@rust-highfive
Copy link
Contributor

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 4, 2021
@Mark-Simulacrum
Copy link
Member

cc @rust-lang/wg-llvm

I'm not personally entirely against this, but I think it may not make sense for us to check this in CI at least unless we can provide a smooth integration path to obtaining clang-format (I guess of the appropriate version).

@cuviper
Copy link
Member

cuviper commented May 4, 2021

I think having .clang-format will be strictly better than the current free-for-all, even if the exact tool version is uncertain.

@Mark-Simulacrum
Copy link
Member

You mean just opportunistically permitting reformats to that style? Yes, that seems reasonably OK to me.

@est31
Copy link
Member Author

est31 commented May 4, 2021

It's only a couple of thousand lines. There shouldn't be that large changes between versions of clang-format.

@nagisa
Copy link
Member

nagisa commented May 4, 2021

If it isn't enforced by the CI, I don't see too much value in an one-off PR to format the code. Eventually the formatting will diverge again. It isn't like we've too atrocious formatting as it is right now, either, right?

Also formatting changes are a pain to git blame through in general, so it better be a one-time ordeal.

@bors
Copy link
Collaborator

bors commented May 9, 2021

☔ The latest upstream changes (presumably #83894) made this pull request unmergeable. Please resolve the merge conflicts.

@est31
Copy link
Member Author

est31 commented May 18, 2021

Hmmm there seems to be little enthusiasm for this change, and I guess it's only 2 thousand lines so whatever.

@est31 est31 closed this May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants