Skip to content

Updating and slightly enhancing instructions for running clang-tidy. #3055

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
Merged
merged 2 commits into from
Jun 22, 2021
Merged

Updating and slightly enhancing instructions for running clang-tidy. #3055

merged 2 commits into from
Jun 22, 2021

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Jun 22, 2021

Follow-on to PR #3051 which updated the docker container version in .github/workflows/format.yml but not in the docs.

Minor clarification from where to run the docker command (for docker muggles like myself).

Adding --yes for convenience and --keep-going for engineering productivity.

Tested interactively.

@rwgk
Copy link
Collaborator Author

rwgk commented Jun 22, 2021

@Skylion007

@rwgk rwgk requested a review from henryiii June 22, 2021 21:15
Skylion007
Skylion007 approved these changes Jun 22, 2021
View reviewed changes
.github/CONTRIBUTING.md
apt-get update && apt-get install python3-dev python3-pytest
cmake -S pybind11/ -B build -DCMAKE_CXX_CLANG_TIDY="$(which clang-tidy);-fix"
cmake --build build
docker run --rm -v $PWD:/mounted_pybind11 -it silkeh/clang:12
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works although docker is a little overkill for clang-tidy IMO. 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Getting a consistent LLVM install on all platforms is not exactly easy. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have a centrally managed "workstation" (in the cloud). Stuff I install manually disappears seemingly randomly. The behavior of a docker container is a bit more predictable.

henryiii
henryiii reviewed Jun 22, 2021
View reviewed changes
@rwgk
Making documented commands identical to workflow commands. Adding com…
13f74af 
…ment in workflow file pointing to documentation.
@rwgk
Copy link
Collaborator Author

rwgk commented Jun 22, 2021

I forgot to mention, the -j 2 is just to tell people "hey you can run this in parallel". That was a question I was uncertain about initially. I think people can take it from there, with the number of the CPUs.

@rwgk
Copy link
Collaborator Author

rwgk commented Jun 22, 2021

Thanks Aaron and Henry!
(I spent my afternoon clang-tidy fixing the smart_holder branch. 😅)

@rwgk rwgk merged commit 484b0f0 into pybind:master Jun 22, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jun 22, 2021
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Jun 22, 2021
@rwgk rwgk deleted the clang-tidy_contrib_doc_update branch June 22, 2021 22:15
@Skylion007
Copy link
Collaborator

@rwgk FYI: You can autofix many clang-tidy issues with the following script: https://clang.llvm.org/extra/doxygen/run-clang-tidy_8py_source.html

@rwgk rwgk mentioned this pull request Feb 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Skylion007 Skylion007 Skylion007 approved these changes

@henryiii henryiii henryiii approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rwgk @Skylion007 @henryiii