Fine tune clang-tidy
#3820
Fine tune clang-tidy
#3820
Conversation
There was a problem hiding this comment.
I think most of these clang-tidy changes add complexity, are misleading, have no effect, or have no meaningful effect.
I don't mind having a clang-tidy action with workflow_dispatch that can advise and be manually reviewed, but developer judgement should be used for changes.
If we have an action for clang-tidy, it should report red only if it fails to run rather than when clang-tidy suggests changes.
| std::span(parent_cell.data(), parent_cell.size())); | ||
| return dolfinx::mesh::MeshTags<std::int32_t>( | ||
| topology1, tdim, std::move(entities), std::move(values)); | ||
| std::move(topology1), tdim, std::move(entities), std::move(values)); |
There was a problem hiding this comment.
std::move on topology1 is unnecessary and will have no effect since MeshTags takes the shared pointer by value.
There was a problem hiding this comment.
The Meshtags argument (taken by value) is then move constructed, not copy constructed. https://godbolt.org/z/zKb3h46qj
cpp/dolfinx/mesh/Topology.cpp
Outdated
| {{{0, 0}, {0, 0}}, | ||
| std::make_shared<graph::AdjacencyList<std::int32_t>>( | ||
| vertex_map->size_local() + vertex_map->num_ghosts())}); | ||
| _index_maps.insert( |
There was a problem hiding this comment.
This change adds complexity for no meaningful performance benefit.
There was a problem hiding this comment.
Initialiser lists are not movable. This removes a somewhat hidden copy of vertex_map.
python/dolfinx/wrappers/fem.cpp
Outdated
|
|
||
| template <typename T> | ||
| void declare_function_space(nb::module_& m, std::string type) | ||
| void declare_function_space(nb::module_& m, const std::string& type) |
There was a problem hiding this comment.
Passing strings by reference isn't really needed in cases like this because (i) the code here is isn't performance critical and (ii) the strings are usually short and compilers have 'short string optimisation'.
I prefer to let the developer make the decision because they know the context.
python/dolfinx/wrappers/fem.cpp
Outdated
| g, std::vector(dofs.data(), dofs.data() + dofs.size()), V); | ||
| std::move(g), | ||
| std::vector(dofs.data(), dofs.data() + dofs.size()), | ||
| std::move(V)); |
There was a problem hiding this comment.
std::move on V not required and will have no effect.
There was a problem hiding this comment.
Moving std::shared_ptr removes the implied overhead by the atomic reference count increase and decrease associated with copying (and deleting) a std::shared_ptr - moving requires no reference count update.
There was a problem hiding this comment.
std::move makes V an rvalue, but the DirichletBC constructor doesn't accept rvalues for the FunctionSpace argument (it takes it by value), which is why it may have no effect unless the compiler can do some copy elision.
Even if it did move the shared pointer, the cost of copying a shared pointer outside a hot loop is trivial. We shouldn't be making code syntactically more complex if there is no measurable performance benefit. Happy to reconsider if a realistic test can demonstrate a meaningful performance difference.
python/dolfinx/wrappers/fem.cpp
Outdated
| std::vector(V_g_dofs[1].data(), | ||
| V_g_dofs[1].data() + V_g_dofs[1].size())}; | ||
| new (bc) dolfinx::fem::DirichletBC(g, std::move(dofs), V); | ||
| new (bc) dolfinx::fem::DirichletBC(std::move(g), std::move(dofs), |
python/dolfinx/wrappers/fem.cpp
Outdated
| new (fp) dolfinx::fem::Form<T, U>( | ||
| spaces, std::move(_integrals), mesh, coefficients, constants, | ||
| needs_permutation_data, ptr_to_ref_wrapper_vec(entity_maps)); | ||
| spaces, std::move(_integrals), std::move(mesh), coefficients, |
There was a problem hiding this comment.
std::move on mesh not needed.
schnellerhase
force-pushed
the
schnellerhase/clang-tidy-2.0
branch
from
3c871bd to
5f35e79
Compare
|
Reverted the changes and changed over to warnings no longer being treated as errors, and running nightly to test clang-tidy works. |
2 participants
Addresses undesired changes introduced in #3722. And reactivates the clang tidy check temporary deactivated in #3814.
const std::shared_ptr<>&are now removed.performance-unnecessary-value-param-check either expects pass by const ref or by value together with move semantics. The later is used throughout, consistent with previous use.nanobind::ndarrayto allow for it to be passed (as non-trivially copyable type) by value and not byconst &.std::stringandstd::filesystem::patharguments. However this would also disallow the checks in other critical parts of the code and it is preferable to stay consistent. Therefore this returns to the initial changes for those in the python exports. If not desired, we could also use another clang-tidy config file for the python exports to only disregard in this section.