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

contrib: Update clang-format-diff #29251

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

TheCharlatan
Copy link
Contributor

@TheCharlatan TheCharlatan commented Jan 15, 2024

This was taken from https://github.com/llvm/llvm-project/blob/900bb318b5b8c485e57cf810253a656b0fb683bc/clang/tools/clang-format/clang-format-diff.py and is useful for systems where clang tools are shipped with a version suffix.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 15, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@TheCharlatan TheCharlatan changed the title contrib: Add --binary option to clang-format-diff contrib: Add -binary option to clang-format-diff Jan 15, 2024
@maflcko
Copy link
Member

maflcko commented Jan 16, 2024

I think you can just pull the latest version?

@TheCharlatan
Copy link
Contributor Author

There were a bunch of changes made to this file to make it "Bitcoin Core" style. Re-applying all of them seems a bit burdensome. Would it be ok to just overwrite these?

@maflcko
Copy link
Member

maflcko commented Jan 16, 2024

Sure. Seems fine to drop them. (If there are any important, you can re-cherry-pick them again on top)?

This was take from
https://github.com/llvm/llvm-project/blob/900bb318b5b8c485e57cf810253a656b0fb683bc/clang/tools/clang-format/clang-format-diff.py

Updating it introduces some new options. For example specifying the
clang-format binary, which is useful for systems where clang tools are
shipped with a version suffix.
@TheCharlatan
Copy link
Contributor Author

@TheCharlatan TheCharlatan changed the title contrib: Add -binary option to clang-format-diff contrib: Update clang-format-diff Jan 17, 2024
@maflcko
Copy link
Member

maflcko commented Jan 17, 2024

You'd have to fixup the linter in one way or another. I think it would be fine to add a commit to modify the script in the two places, or add an exclusion, or something else.

We assume to be using python3, so don't check for it. This removes a
type error on the line `from io import BytesIO as StringIO`.

Specify the encoding as "utf8" when opening a file.
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/20563036485

@TheCharlatan
Copy link
Contributor Author

Updated 008e81e -> 52149b7 (ClangFormatDiffBinaryOption_1 -> ClangFormatDiffBinaryOption_2, compare)

  • Added a commit fixing the lint errors by removing a python2 check and specifying "utf8" encoding when opening a file.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

lgtm ACK 52149b7 🌱

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 52149b7a2c2b48ed4a4c0900c74cda4bb52a1ea5  🌱
OvfYlA4NC+vgQi+PK7fancIkRgGFUMoF0GnyeDigp2SjZhMD7QeiPlg8jfTtivZi0xWiOM2gSMUV1FNgPS2pCg==

@fanquake fanquake merged commit 3d52ced into bitcoin:master Jan 17, 2024
15 of 16 checks passed
@TheCharlatan TheCharlatan mentioned this pull request Jun 3, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants