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.

Sign up for GitHub

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

Jump to bottom

[run-clang-tidy,clang-tidy-diff] Accept directory as value for -export-fixes #69453

Merged
merged 2 commits into from
Oct 20, 2023
Merged

[run-clang-tidy,clang-tidy-diff] Accept directory as value for -export-fixes #69453

merged 2 commits into from
Oct 20, 2023

Conversation

amgebauer
Copy link
Contributor

@amgebauer amgebauer commented Oct 18, 2023 •
edited
Loading

Adding an additional parameter to run_clang_tidy.py to accept a directory where the clang-tidy fixes are saved to. This directory can then be used to run clang-apply-replacements.

Closes #69450

@PiotrZSL
Copy link
Member

There is already --export_fixes that can be used in +- same way.
Maybe better would be to change --export_fixes to take filename or directory (ends with / or exist as directory), and in that case it could save to directory or merge to single file.

@amgebauer amgebauer force-pushed the 69450-run-clang-tidy-accepts-export-directory branch from fd908df to d63de8e Compare October 18, 2023 14:05
@amgebauer
Copy link
Contributor Author

@PiotrZSL Thank you for your feedback. I implemented it the way you suggest. Feel free to leave comments.

@amgebauer amgebauer changed the title [run-clang-tidy] Add option -export-directory [run-clang-tidy] Accept directory as value for -export-fixes Oct 18, 2023
@github-actions
Copy link

github-actions bot commented Oct 18, 2023 •
edited
Loading

✅ With the latest revision this PR passed the Python code formatter.

PiotrZSL
PiotrZSL reviewed Oct 18, 2023
View reviewed changes
Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

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

Release notes will be needed, in clang-tools-extra/docs/ReleaseNotes.rst.
Additionally probably similar change should be done to clang-tidy-diff.py

@amgebauer amgebauer force-pushed the 69450-run-clang-tidy-accepts-export-directory branch from d63de8e to 00b993e Compare October 18, 2023 14:53
@amgebauer amgebauer changed the title [run-clang-tidy] Accept directory as value for -export-fixes [run-clang-tidy,clang-tidy-diff] Accept directory as value for -export-fixes Oct 18, 2023
PiotrZSL
PiotrZSL approved these changes Oct 18, 2023
View reviewed changes
Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

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

LGTM.
Would be good to test if both scripts are working before merge.

@amgebauer amgebauer force-pushed the 69450-run-clang-tidy-accepts-export-directory branch from 00b993e to 9544ce9 Compare October 18, 2023 15:50
@amgebauer amgebauer force-pushed the 69450-run-clang-tidy-accepts-export-directory branch from 9544ce9 to 1001c25 Compare October 18, 2023 16:27
@amgebauer
Copy link
Contributor Author

amgebauer commented Oct 18, 2023 •
edited
Loading

I tested both scripts locally with both options (providing a yaml file and a directory) and both work on my machine.

@amgebauer
Copy link
Contributor Author

From my side it is ready to be merged. I don't have the access rights to do so.

@PiotrZSL PiotrZSL merged commit 5557d98 into llvm:main Oct 20, 2023
@dyung
Copy link
Collaborator

dyung commented Oct 20, 2023

Hi @amgebauer, not sure if you got the notification, but your change seems to be causing a test failure on two build bots, any idea why?
https://lab.llvm.org/buildbot/#/builders/139/builds/51852
https://lab.llvm.org/buildbot/#/builders/247/builds/10286

@amgebauer
Copy link
Contributor Author

I have an idea about the reason. I will push a fix in a few moments. Thank you for the hint

@amgebauer amgebauer mentioned this pull request Oct 20, 2023
Merged
PiotrZSL pushed a commit that referenced this pull request Oct 20, 2023
@amgebauer
[run-clang-tidy] Accept export directory if PyYAML is not installed (#…
49af650 
…69700)

If PyYAML is not installed, the `-export-fixes` can be used to specify a
directory (not a file).

Mentioning @PiotrZSL @dyung 

Follows #69453
@madhur13490 madhur13490 mentioned this pull request Oct 20, 2023
Closed
@spupyrev spupyrev mentioned this pull request Oct 23, 2023
Merged
@banach-space banach-space mentioned this pull request Oct 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PiotrZSL PiotrZSL PiotrZSL approved these changes

Assignees
No one assigned
Labels
clang-tidy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[clang-tidy] run_clang_tidy.py should accept a directory to export clang-tidy fixes
4 participants
@amgebauer @PiotrZSL @dyung @llvmbot