fix: Clazy warnings

[gruenich]

This saves copy-ctor and dtor for non-trivial types by value
Found by clazy (range-loop-reference)

Suggested changelog entry:

Minor cleanup from warnings reported by Clazy

[Skylion007]

Is this actually necessary with auto? I think this is qualifying the auto call? It should be a const ref regardless right, unless I am missing something? We sure this isn't a false positive with clazy?

[gruenich]

Other people suggest const auto&, too. https://stackoverflow.com/a/15176127/2799037
It might be too much, but it does no harm.

[rwgk]

Seems fine to me, even though technically it's probably entirely inconsequential. The explicit const & make the intent clear without a doubt, and if it silences clazy, it removes a distraction for someone reviewing clazy results.

[rwgk]

I forgot to ask: Are there any other clazy warnings?

[gruenich]

I forgot to ask: Are there any other clazy warnings?

Good question! With a minimal set of dependencies I only get two other warnings

/home/gruenich/pybind11/tests/test_smart_ptr.cpp:109:1: warning: non-POD static (unordered_set) [-Wclazy-non-pod-global-static]
  109 | std::unordered_set<MyObject4 *> myobject4_instances;
      | ^
/home/gruenich/pybind11/tests/test_smart_ptr.cpp:137:1: warning: non-POD static (unordered_set) [-Wclazy-non-pod-global-static]
  137 | std::unordered_set<MyObject4a *> myobject4a_instances;
      | ^
2 warnings generated.

Documentation for this warning: https://github.com/KDE/clazy/blob/master/docs/checks/README-non-pod-global-static.md

[rwgk]

Good question! With a minimal set of dependencies I only get two other warnings

Indeed, it would be better to leak those:

• https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

Objects with static storage duration are forbidden unless they are trivially destructible.

Questions:

• Do you want to add the test_smart_ptr.cpp change to this PR? With the idea that you don't get any pybind11-related clazy warnings anymore.

• How much trouble is it to add a pybind11 GitHub Actions workflow to run clazy similar to what you did manually? Do you think it's worth it? (I think it's worth it only if it's very easy.)

[gruenich]

Do you want to add the test_smart_ptr.cpp change to this PR? With the
idea that you don't get any pybind11-related clazy warnings anymore.

Having this fixed is the right approach. I am unsure how the proper change would look like. 😇

How much trouble is it to add a pybind11 GitHub Actions workflow
to run clazy similar to what you did manually? Do you think it's
worth it? (I think it's worth it only if it's very easy.)

Using clazy is super easy, just install the package and use clazy as the compiler (add -DCMAKE_CXX_COMPILER=clazy to the CMake call). I don't know if it is easy to display the results. Nevertheless, clazy aims to find Qt-related bugs. For pybind11 I would start with Cppcheck or clang-tidy, as the CPU cycles spent result in more suitable findings.

[gruenich]

I figured that any code change for the test code would make it less readable and confuse future developers. Thus I explicitly suppressed the second warning.
I still doubt that adding a Clazy GitHub action would be the right thing. But if you insist, I am willing to provide a commit doing so.

[rwgk]

Could you please try #4993? Does that fix the clazy warnings for test_smart_ptr.cpp?

[rwgk]

I spent some time looking for a clang-tidy equivalent of clazy's non-pod-global-static but didn't get lucky. Giving up. If anyone know, could you please let me know?

[gruenich]

I tried your patch, the non-POD warning keeps coming.

I spent some time looking for a clang-tidy equivalent of clazy's non-pod-global-static but didn't get lucky.

Clazy only provides checks that are not part of clang-tidy. A quote from another check: "[The skipped-base-method] check might get removed in the future, as clang-tidy recently got a similar feature."

[rwgk]

I tried your patch,

Thanks!

the non-POD warning keeps coming.

Oh ... but it actually makes sense. The new trick only removes destruction order issues.

Could you please try again: 06475c9

(That's the pattern I usually follow in production code.)

[rwgk]

I forgot to mention: previously I accidentally lost the static (static auto ...), but I guess just adding that back doesn't resolve the clazy warning.

[gruenich]

This did the trick, the Clazy warnings are gone! I squashed your two commits and added the resulting one to this pull request.

[rwgk]

Merged, thanks!

The non-pod-global-static check seems very useful, I hope it'll get ported to clang-tidy in the future.