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

[clang-apply-replacements] Apply format only if --format is specified #70801

Closed

Conversation

kuganv
Copy link
Contributor

@kuganv kuganv commented Oct 31, 2023

clang-apply-replacements is now applying format even when --format is not specified. Methods like createReplacementsForHeaders only takes the Spec.Style and would re-order the headers even when it was not requested. The fix is to set up Spec.Style only if --format is provided.

@kuganv kuganv changed the title Apply format only if --format is specified [clang-apply-replacements] Apply format only if --format is specified Oct 31, 2023
@kuganv kuganv marked this pull request as draft October 31, 2023 21:17
@kuganv kuganv marked this pull request as ready for review October 31, 2023 21:18
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Can you please add a testcase?

clang-apply-replacements is now applying format even when --format is specified.  This because, methods like createReplacementsForHeaders only takes the Spec.Style and would re-order the headers even when it was not requested. The fix is to set up Spec.Style only if --format is provided.
@kuganv kuganv force-pushed the Fix-clang-apply-replacements-format-issue branch from 3fd4b8e to ac5aae4 Compare November 16, 2023 14:56
@kuganv kuganv marked this pull request as draft November 20, 2023 11:21
@kuganv kuganv marked this pull request as ready for review November 20, 2023 11:24
@kuganv
Copy link
Contributor Author

kuganv commented Nov 20, 2023

@revane, @pepsiman: Could you please review this. Thanks.

@revane
Copy link
Contributor

revane commented Nov 20, 2023

Does this PR address a bug I logged? I'm not sure why you've requested my input.

@kuganv
Copy link
Contributor Author

kuganv commented Nov 20, 2023

Does this PR address a bug I logged? I'm not sure why you've requested my input.

Sorry for not being clear. I was tagging developers who touched this part of the codebase to see if you can review this change.

@revane
Copy link
Contributor

revane commented Nov 20, 2023

I haven't touched this code in 10 years. I don't think I'm a good candidate for providing a review.

@pepsiman
Copy link
Contributor

createReplacementsForHeaders() is applying a cleanup without checking the Cleanup flag.
But clang-apply-replacements always sets Cleanup to true anyway.
The change looks good to me, but it might be working around a bug elsewhere.

@kuganv
Copy link
Contributor Author

kuganv commented Dec 11, 2023

ping @AaronBallman ?

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

The changes LGTM but we should probably add a release note to https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/docs/ReleaseNotes.rst?plain=1 so users know about the fix.

@dmpolukhin
Copy link
Contributor

@kuganv do you have cycles to work on this issue? If not, I can commandeer this pull requests and update release notes. Please let me know what is your preference.

dmpolukhin pushed a commit to dmpolukhin/llvm-project that referenced this pull request Jan 25, 2024
clang-apply-replacements used to apply format even without --format is specified. This because, methods like createReplacementsForHeaders only takes the Spec.Style and would re-order the headers even when it was not requested. The fix is to set up Spec.Style only if --format is provided.

Also added note to ReleaseNotes.rst

Based on llvm#70801
dmpolukhin added a commit that referenced this pull request Feb 5, 2024
clang-apply-replacements used to apply format even without --format is
specified. This because, methods like createReplacementsForHeaders only
takes the Spec.Style and would re-order the headers even when it was not
requested. The fix is to set up Spec.Style only if --format is provided.

Also added note to ReleaseNotes.rst

Based on #70801

---------

Co-authored-by: Kugan <34810920+kuganv@users.noreply.github.com>
Co-authored-by: Aaron Ballman <aaron@aaronballman.com>
@dmpolukhin dmpolukhin closed this Feb 5, 2024
@dmpolukhin
Copy link
Contributor

Pushed as #79466

agozillon pushed a commit to agozillon/llvm-project that referenced this pull request Feb 5, 2024
clang-apply-replacements used to apply format even without --format is
specified. This because, methods like createReplacementsForHeaders only
takes the Spec.Style and would re-order the headers even when it was not
requested. The fix is to set up Spec.Style only if --format is provided.

Also added note to ReleaseNotes.rst

Based on llvm#70801

---------

Co-authored-by: Kugan <34810920+kuganv@users.noreply.github.com>
Co-authored-by: Aaron Ballman <aaron@aaronballman.com>
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.

None yet

6 participants