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

Docs: Update py::kwargs example in function.rst to pass by reference #3038

Merged
merged 1 commit into from
Jun 17, 2021

Conversation

Skylion007
Copy link
Collaborator

Description

A very common clang-tidy performance issue I have noticed is that py::dict is not trivially copyable, but I can't think of any case where where you want to actually create a shallow copy of the dict intentionally. Furthermore, not creating a shallow-copy of the dict would better match pythonic syntax in the C++ side. As such, I propose editing the documentation to have this be the default practice. There are probably a lot more areas where this could be improved, but here are the two most common pieces of code I see being copy and pasted around.

As an aside, I'd recommend enabling the clang-tidy performance checks at some point to catch these issues in the tests and other places. Not that they are performance sensitive per say, but that they should show best coding practices.

Suggested changelog entry:

* Improved documentation for best practices regarding ``py::dict`` and ``py::kwargs``

@rwgk
Copy link
Collaborator

rwgk commented Jun 17, 2021

The Centos 8 failure is unrelated (it appeared ~June 4; we're working on it).

@rwgk rwgk merged commit cd4b49a into pybind:master Jun 17, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jun 17, 2021
@rwgk rwgk added needs changelog Possibly needs a changelog entry and removed needs changelog Possibly needs a changelog entry labels Jun 17, 2021
@rwgk
Copy link
Collaborator

rwgk commented Jun 17, 2021

Thanks for this change!

I really like your clang-tidy suggestion, but unless we find a volunteer to add this to the GitHub Actions it's very unlikely to happen.

@Skylion007
Copy link
Collaborator Author

@rwgk Happy to do it, but the only way I know how to do to it properly is to have a hierarchical config (since we don't want it making changes to the obvious copies and movies that we do in the tests folder). These would require a new version of Clang-Tidy (>12) and the github action only uses clang-tidy 10. Happy to set this up if you don't mind bumping the clang-tidy version: https://reviews.llvm.org/D75184

@rwgk
Copy link
Collaborator

rwgk commented Jun 17, 2021

@rwgk Happy to do it, but the only way I know how to do to it properly is to have a hierarchical config (since we don't want it making changes to the obvious copies and movies that we do in the tests folder). These would require a new version of Clang-Tidy (>12) and the github action only uses clang-tidy 10. Happy to set this up if you don't mind bumping the clang-tidy version: https://reviews.llvm.org/D75184

I'm up for trying, assuming we can roll back to 10 if trying 12 gets us into trouble.
@henryiii to you have an opinion?

I'm not very familiar with the GitHub Action tooling. Is bumping the clang-tidy version just another PR? In that case I'd say let's just try and make decisions based on the outcome.

@Skylion007
Copy link
Collaborator Author

@rwgk In the meantime, I applied the performance optimizations from clang-tidy in another PR: #3046 . There were not too many (as expected of a well optimized codebase), but there were several on relatively hot code paths including arg_v, static_properties and such.

@henryiii
Copy link
Collaborator

Sure, feel free to bump

container: silkeh/clang:10
to any supported number. I can do that if you prefer.

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 13, 2021
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

3 participants