Cleanup clang tidy #882

mgovers · 2025-01-27T08:12:35Z

Ensures clang-tidy runs on Visual Studio 2022 latest version.

Relates to #267 .

Note: I had to go from std::ranges::copy(foo, std::back_inserter(bar)); to bar.insert(bar.end(), foo.begin(), foo.end()); due to llvm/llvm-project#120151 . I prefer to keep using the former but I first wanted to do a full build

Signed-off-by: Martijn Govers <[email protected]>

…um degree Signed-off-by: Martijn Govers <[email protected]>

Signed-off-by: Martijn Govers <[email protected]>

mgovers · 2025-01-27T08:14:58Z

power_grid_model_c/power_grid_model/include/power_grid_model/common/common.hpp

@@ -93,7 +93,10 @@ concept is_in_list_c = (std::same_as<std::remove_const_t<T>, Ts> || ...);

 // functor to include all
 struct IncludeAll {
-    template <class... T> constexpr bool operator()(T&&... /*ignored*/) const { return true; }
+    template <class... T> constexpr bool operator()(T&&... ignored) const {
+        ((void)std::forward<T>(ignored), ...);


here and below: the capture is correct: this should work on any type and zero-copy, regardless of whether it is a value, an lvalue or a prvalue. However, there is nothing to forward to, so it must be a (void) sink

Why are you doing this instead of the original? Have a unnamed argument will not trigger an unused argument warning.

After re-thinking about it, the original implementation is not correct. We do not do anything with the argument so it does not make sense to make it universal reference. Just define as unnamed const lvalue reference and throw them away.

Suggested change

((void)std::forward<T>(ignored), ...);

template <class... T> constexpr bool operator()(T const&... /*ignored*/) const { return true; }

mgovers · 2025-01-27T08:17:42Z

power_grid_model_c/power_grid_model/include/power_grid_model/common/three_phase_tensor.hpp

+                current_value(i) = scalar * new_value(i); // can't forward due to runtime element access
            }
+            (void)std::forward<Proxy>(current_value);


runtime access; can't move in the for loop. Therefore, either the sink is necessary, or it is required to use (symbolically) for (i : {0, 1}) current_value(i) = a(i); std::forward<Proxy>(current_value)(2) = a(2) but that is way more unreadable

see #882 (comment)

Signed-off-by: Martijn Govers <[email protected]>

sonarqubecloud · 2025-01-28T12:20:12Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

TonyXiang8787 · 2025-02-01T14:26:04Z

power_grid_model_c/power_grid_model/include/power_grid_model/topology.hpp

@@ -530,6 +530,7 @@ class Topology {
                coupling[topo_comp_i] = Idx2D{topo_idx, new_math_comp_i};
            }
        }
+        (void)std::forward<GetMathTopoComponent>(get_component_topo);


why is this happening?

this function captures get_component_topo by perfect forwarding. It is not allowed by the standard to not forward a forwarded argument, or move a moved argument.

See #882 (comment).

The argument get_component_topo should be passed by value as lambdas.

TonyXiang8787 · 2025-02-01T14:26:41Z

power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/sparse_lu_solver.hpp

@@ -138,6 +138,7 @@ template <rk2_tensor Matrix> class DenseLUFactor {
                throw SparseMatrixError{}; // can not specify error code
            }
        }
+        (void)std::move(matrix);


this function captures matrix by prvalue. It is not allowed by the standard to not move a moved argument, or forward a forwarded argument.

What do you mean not allowed by standard? I do not think standard forbid you to capture a rvalue but not to forward or move them. It is just a (possibly false) warning from clang-tidy right?

TonyXiang8787 · 2025-02-01T17:39:06Z

...id_model_c/power_grid_model/include/power_grid_model/math_solver/common_solver_functions.hpp

@@ -167,6 +167,7 @@ inline void calculate_load_gen_result(IdxRange const& load_gens, Idx bus_number,
        }
        output.load_gen[load_gen].i = conj(output.load_gen[load_gen].s / output.u[bus_number]);
    }
+    (void)std::forward<LoadGenFunc>(load_gen_func);


this function captures load_gen_func by perfect forwarding. It is not allowed by the standard to not forward a forwarded argument, or move a moved argument.

This and the other for file common_solver_functions.hpp, the root cause is we define the argument for the lambda as LoadGenFunc&& load_gen_func, which is not correct.

Lambdas in function argument should be passed by value unless there is a strong reason of not to do.

If we change that to LoadGenFunc load_gen_func. The stuff will go away.

TonyXiang8787 · 2025-02-01T17:39:16Z

...id_model_c/power_grid_model/include/power_grid_model/math_solver/common_solver_functions.hpp

@@ -191,6 +192,7 @@ inline void calculate_pf_result(YBus<sym> const& y_bus, PowerFlowInput<sym> cons
        calculate_load_gen_result<sym>(load_gens, bus_number, input, output, load_gen_func);
        calculate_source_result<sym>(sources, bus_number, y_bus, input, output, load_gens);
    }
+    (void)std::forward<LoadGenFunc>(load_gen_func);


this function captures load_gen_func by perfect forwarding. It is not allowed by the standard to not forward a forwarded argument, or move a moved argument.

See #882 (comment)

TonyXiang8787 · 2025-02-01T17:39:34Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/update.hpp

@@ -29,6 +29,7 @@ inline void iterate_component_sequence(Func&& func, ForwardIterator begin, Forwa
        // get component directly using sequence id
        func(*it, sequence_idx[seq]);
    }
+    (void)std::forward<Func>(func);


this function captures func by perfect forwarding. It is not allowed by the standard to not forward a forwarded argument, or move a moved argument.

Same here for #882 (comment).

func should be defined as pass by value in the function argument.

TonyXiang8787 · 2025-02-01T17:40:24Z

power_grid_model_c/power_grid_model/include/power_grid_model/common/three_phase_tensor.hpp

@@ -281,7 +281,7 @@ inline void add_diag(Eigen::ArrayBase<DerivedA>& x, Eigen::ArrayBase<DerivedB> c
 }
 template <rk2_tensor DerivedA, column_vector DerivedB>
 inline void add_diag(Eigen::ArrayBase<DerivedA>&& x, Eigen::ArrayBase<DerivedB> const& y) {
-    x.matrix().diagonal() += y.matrix();
+    std::move(x).matrix().diagonal() += y.matrix();


this is very strange

this is not a perfect forwarding because it contains Eigen::ArrayBase<DerivedA> (a deduced type) instead of template A directly. Therefore, it creates a view by value, which is then passed into this function as an rvalue and therefore, it has to be moved.

Alternatively, we can decide to not capture by && but by value, but I don't know whether that is how Eigen works, I would need to dig into it.

Now this (and other places) is indeed a nasty one. EIGEN uses expression template extensively, which is basically a (temporary) proxy object.

For this function, we have a rvalue overload because the caller can do something like:

add_diag(real(complex_tensor), diag_value);

Where real(complex_tensor) returns a temporary rvalue of EIGEN expression template, so the other lvalue overload will not work. This is what I call the third use-case of rvalue reference in the function argument: move, forward, and capture temporary mutable proxy object. The clang-tidy does not correctly recognize the third use-case of rvalue reference and raises a false warning.

We can silence the warning locally. We can also choose to define a named variable in all the caller side, so

auto& real_value_proxy = real(complex_tensor); add_diag(real_value_proxy , diag_value);

Then we just need the lvalue overload for the function. This also applies to other parts.

TonyXiang8787 · 2025-02-01T17:40:35Z

power_grid_model_c/power_grid_model/include/power_grid_model/common/three_phase_tensor.hpp

@@ -351,13 +351,14 @@ template <symmetry_tag sym, class Proxy>
 inline void update_real_value(RealValue<sym> const& new_value, Proxy&& current_value, double scalar) {
    if constexpr (is_symmetric_v<sym>) {
        if (!is_nan(new_value)) {
-            current_value = scalar * new_value;
+            std::forward<Proxy>(current_value) = scalar * new_value;


also this seems to be very strange.

this is how a perfectly forwarded proxy works: the current_value is either taken by & or by && or by value depending on what the type in caller is, and then here it is called as

std::forward<Proxy>.operator=(scalar * new_value);

which can be rewritten as std::forward<Proxy>(current_value) = scalar * new_value;

This deliberately consumes the current_value in the case of forwarding on a prvalue.

If we do not want to do perfect forwarding, can you please explain why we went for this solution in the first place here? I do understand that this implementation creates a modifiable view of the 3-phase tensor by value, while taking mutable reference for other objects, but it this really the way that Eigen wants us to do it?

see #882 (comment)

TonyXiang8787

@mgovers please look very critically for artificial void, move, and forward.

I strongly object the idea to use those artificially just to get clang-tidy happy. If we have a considered choice to ignore the warning, we should either silence it globally or locally, or use an unnamed argument.

TonyXiang8787

@mgovers please look very critically for artificial void, move, and forward.

I strongly object the idea to use those artificially just to get clang-tidy happy. If we have a considered choice to ignore the warning, we should either silence it globally or locally, or use an unnamed argument.

mgovers · 2025-02-03T07:12:41Z

@mgovers please look very critically for artificial void, move, and forward.

I strongly object the idea to use those artificially just to get clang-tidy happy. If we have a considered choice to ignore the warning, we should either silence it globally or locally, or use an unnamed argument.

on the eigen things, we should really have a look whether this is the correct way to capture views on Eigen matrices. On the function versions, the standard tells us that we should use the & version of calling a function on all except the last call. in the last call, the function should be perfectly forwarded into its next call. it looks ugly, I know, but it's the only correct usage.

if we really need to we should silence the warning locally, but i'm very hesitant to disable it on a global level because it usually really is a bug to forget a forward or move

TonyXiang8787 · 2025-02-03T08:54:18Z

@mgovers please look very critically for artificial void, move, and forward.
I strongly object the idea to use those artificially just to get clang-tidy happy. If we have a considered choice to ignore the warning, we should either silence it globally or locally, or use an unnamed argument.

on the eigen things, we should really have a look whether this is the correct way to capture views on Eigen matrices. On the function versions, the standard tells us that we should use the & version of calling a function on all except the last call. in the last call, the function should be perfectly forwarded into its next call. it looks ugly, I know, but it's the only correct usage.

if we really need to we should silence the warning locally, but i'm very hesitant to disable it on a global level because it usually really is a bug to forget a forward or move

To summarize, there are two major things here:

Lambda argument wrongly defined as rvalue. They should be passed by value. The problems go away.
EIGEN expression stuff. We can either silence the warning locally, or define a named variable at caller side.

mgovers added 5 commits January 24, 2025 09:36

clang-tidy

133e334

Signed-off-by: Martijn Govers <[email protected]>

more clang-tidy stuff

0f8a9d5

Signed-off-by: Martijn Govers <[email protected]>

solve more clang-tidy

3909008

Signed-off-by: Martijn Govers <[email protected]>

clang-tidy-19 visual studio version passes on unit tests except minim…

862dffd

…um degree Signed-off-by: Martijn Govers <[email protected]>

clang-tidy back_inserter issue

af6a46e

Signed-off-by: Martijn Govers <[email protected]>

mgovers added the improvement Improvement on internal implementation label Jan 27, 2025

mgovers self-assigned this Jan 27, 2025

mgovers added 2 commits January 27, 2025 09:46

resolve

a9799ff

Signed-off-by: Martijn Govers <[email protected]>

more serializer improvements

ff5638c

Signed-off-by: Martijn Govers <[email protected]>

mgovers commented Jan 27, 2025

View reviewed changes

mgovers and others added 8 commits January 27, 2025 10:08

minor

762160d

Signed-off-by: Martijn Govers <[email protected]>

Merge branch 'main' into feature/clang-tidy

cca35b0

minor

9c136ad

Signed-off-by: Martijn Govers <[email protected]>

minor

75b8802

Signed-off-by: Martijn Govers <[email protected]>

remove deprecated

8d594ce

Signed-off-by: Martijn Govers <[email protected]>

main model also compiles with latest visual studio clang-tidy

a9a560a

Signed-off-by: Martijn Govers <[email protected]>

sonar cloud

0ed9ac1

Signed-off-by: Martijn Govers <[email protected]>

Merge branch 'main' into feature/clang-tidy

10937a8

mgovers marked this pull request as ready for review January 27, 2025 13:26

mgovers changed the title ~~Feature/clang tidy~~ Cleanup clang tidy Jan 28, 2025

Merge branch 'main' into feature/clang-tidy

423ff5b

TonyXiang8787 reviewed Feb 1, 2025

View reviewed changes

TonyXiang8787 approved these changes Feb 1, 2025

View reviewed changes

TonyXiang8787 requested changes Feb 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup clang tidy #882

Cleanup clang tidy #882

mgovers commented Jan 27, 2025

mgovers Jan 27, 2025

TonyXiang8787 Feb 1, 2025 •

edited

Loading

TonyXiang8787 Feb 3, 2025

mgovers Jan 27, 2025

TonyXiang8787 Feb 3, 2025

sonarqubecloud bot commented Jan 28, 2025

TonyXiang8787 Feb 1, 2025

mgovers Feb 3, 2025 •

edited

Loading

TonyXiang8787 Feb 3, 2025

TonyXiang8787 Feb 1, 2025

mgovers Feb 3, 2025 •

edited

Loading

TonyXiang8787 Feb 3, 2025

TonyXiang8787 Feb 1, 2025

mgovers Feb 3, 2025 •

edited

Loading

TonyXiang8787 Feb 3, 2025

TonyXiang8787 Feb 1, 2025

mgovers Feb 3, 2025 •

edited

Loading

TonyXiang8787 Feb 3, 2025

TonyXiang8787 Feb 1, 2025

mgovers Feb 3, 2025

TonyXiang8787 Feb 3, 2025

TonyXiang8787 Feb 1, 2025

mgovers Feb 3, 2025

TonyXiang8787 Feb 3, 2025 •

edited

Loading

TonyXiang8787 Feb 1, 2025

mgovers Feb 3, 2025

TonyXiang8787 Feb 3, 2025

TonyXiang8787 left a comment

TonyXiang8787 left a comment

mgovers commented Feb 3, 2025 •

edited

Loading

TonyXiang8787 commented Feb 3, 2025

	((void)std::forward<T>(ignored), ...);
	template <class... T> constexpr bool operator()(T const&... /ignored/) const { return true; }

Cleanup clang tidy #882

Are you sure you want to change the base?

Cleanup clang tidy #882

Conversation

mgovers commented Jan 27, 2025

Choose a reason for hiding this comment

TonyXiang8787 Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 28, 2025

Quality Gate passed

Choose a reason for hiding this comment

mgovers Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgovers Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgovers Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgovers Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyXiang8787 Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyXiang8787 left a comment

Choose a reason for hiding this comment

TonyXiang8787 left a comment

Choose a reason for hiding this comment

mgovers commented Feb 3, 2025 • edited Loading

TonyXiang8787 commented Feb 3, 2025

TonyXiang8787 Feb 1, 2025 •

edited

Loading

mgovers Feb 3, 2025 •

edited

Loading

mgovers Feb 3, 2025 •

edited

Loading

mgovers Feb 3, 2025 •

edited

Loading

mgovers Feb 3, 2025 •

edited

Loading

TonyXiang8787 Feb 3, 2025 •

edited

Loading

mgovers commented Feb 3, 2025 •

edited

Loading