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.

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

Cleanup clang tidy #882

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Cleanup clang tidy #882

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
133e334
clang-tidy
mgovers Jan 24, 2025
0f8a9d5
more clang-tidy stuff
mgovers Jan 24, 2025
3909008
solve more clang-tidy
mgovers Jan 24, 2025
862dffd
clang-tidy-19 visual studio version passes on unit tests except minim…
mgovers Jan 24, 2025
af6a46e
clang-tidy back_inserter issue
mgovers Jan 27, 2025
a9799ff
resolve
mgovers Jan 27, 2025
ff5638c
more serializer improvements
mgovers Jan 27, 2025
762160d
minor
mgovers Jan 27, 2025
cca35b0
Merge branch 'main' into feature/clang-tidy
mgovers Jan 27, 2025
9c136ad
minor
mgovers Jan 27, 2025
75b8802
minor
mgovers Jan 27, 2025
8d594ce
remove deprecated
mgovers Jan 27, 2025
a9a560a
main model also compiles with latest visual studio clang-tidy
mgovers Jan 27, 2025
0ed9ac1
sonar cloud
mgovers Jan 27, 2025
10937a8
Merge branch 'main' into feature/clang-tidy
mgovers Jan 27, 2025
423ff5b
Merge branch 'main' into feature/clang-tidy
mgovers Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions ...odel_c/power_grid_model/include/power_grid_model/auxiliary/serialization/deserializer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ using nlohmann::json;
// visitor for json conversion
struct JsonMapArrayData {
size_t size{};
msgpack::sbuffer buffer{};
msgpack::sbuffer buffer;
};

struct JsonSAXVisitor {
Expand Down Expand Up @@ -130,8 +130,8 @@ struct JsonSAXVisitor {
throw SerializationError{ss.str()};
}

std::stack<JsonMapArrayData> data_buffers{};
msgpack::sbuffer root_buffer{};
std::stack<JsonMapArrayData> data_buffers;
msgpack::sbuffer root_buffer;
};

// visitors for parsing
Expand Down Expand Up @@ -192,8 +192,15 @@ struct MapArrayVisitor : DefaultErrorVisitor<MapArrayVisitor<map_array>> {
std::same_as<map_array, visit_map_t> || std::same_as<map_array, visit_map_array_t>;
static constexpr bool enable_array =
std::same_as<map_array, visit_array_t> || std::same_as<map_array, visit_map_array_t>;
static constexpr std::string_view static_err_msg =
enable_map ? (enable_array ? "Map or an array expected." : "Map expected.") : "Array expected.";
static constexpr std::string_view static_err_msg = [] {
if constexpr (enable_map && enable_array) {
return "Map or an array expected.";
} else if constexpr (enable_map) {
return "Map expected.";
} else {
return "Array expected.";
}
}();

Idx size{};
bool is_map{};
Expand Down Expand Up @@ -228,7 +235,7 @@ struct MapArrayVisitor : DefaultErrorVisitor<MapArrayVisitor<map_array>> {
struct StringVisitor : DefaultErrorVisitor<StringVisitor> {
static constexpr std::string_view static_err_msg = "String expected.";

std::string_view str{};
std::string_view str;
bool visit_str(const char* v, uint32_t size) {
str = {v, size};
return true;
Expand Down
11 changes: 5 additions & 6 deletions ..._model_c/power_grid_model/include/power_grid_model/auxiliary/serialization/serializer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ struct JsonConverter : msgpack::null_visitor {

Idx indent;
Idx max_indent_level;
std::stringstream ss{};
std::stack<MapArray> map_array{};
std::stringstream ss{}; // NOLINT(readability-redundant-member-init) // libc++ requires it
std::stack<MapArray> map_array;

void print_indent() {
if (indent < 0) {
Expand Down Expand Up @@ -293,7 +293,7 @@ class Serializer {
std::vector<ComponentBuffer> component_buffers_; // list of components, then all scenario flatten

// msgpack pakcer
msgpack::sbuffer msgpack_buffer_{};
msgpack::sbuffer msgpack_buffer_;
msgpack::packer<msgpack::sbuffer> packer_;
bool use_compact_list_{};
std::map<MetaComponent const*, std::vector<MetaAttribute const*>> attributes_;
Expand Down Expand Up @@ -606,9 +606,8 @@ class Serializer {
});
}
void pack_attribute(AttributeBuffer<void const> const& attribute_buffer, Idx idx) {
return ctype_func_selector(attribute_buffer.meta_attribute->ctype, [&]<class T> {
packer_.pack(*(reinterpret_cast<T const*>(attribute_buffer.data) + idx));
});
ctype_func_selector(attribute_buffer.meta_attribute->ctype,
[&]<class T> { packer_.pack(*(reinterpret_cast<T const*>(attribute_buffer.data) + idx)); });
}

static constexpr BufferView advance(BufferView buffer_view, Idx offset) {
Expand Down
6 changes: 3 additions & 3 deletions power_grid_model_c/power_grid_model/include/power_grid_model/calculation_parameters.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ template <symmetry_tag sym_type> struct BranchShortCircuitSolverOutput {

// fault math calculation parameters and math output
struct FaultCalcParam {
DoubleComplex y_fault{};
DoubleComplex y_fault;
FaultType fault_type{};
FaultPhase fault_phase{};
};
Expand Down Expand Up @@ -136,7 +136,7 @@ static_assert(sensor_calc_param_type<PowerSensorCalcParam<asymmetric_t>>);
struct TransformerTapRegulatorCalcParam {
double u_set{};
double u_band{};
DoubleComplex z_compensation{};
DoubleComplex z_compensation;
IntS status{};
};

Expand Down Expand Up @@ -203,7 +203,7 @@ struct SourceCalcParam {
DoubleComplex y1;
DoubleComplex y0;

template <symmetry_tag sym> inline ComplexTensor<sym> y_ref() const {
template <symmetry_tag sym> ComplexTensor<sym> y_ref() const {
if constexpr (is_symmetric_v<sym>) {
return y1;
} else {
Expand Down
5 changes: 4 additions & 1 deletion power_grid_model_c/power_grid_model/include/power_grid_model/common/common.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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), ...);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@TonyXiang8787 TonyXiang8787 Feb 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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; }

return true;
}
};
constexpr IncludeAll include_all{};

Expand Down
2 changes: 1 addition & 1 deletion power_grid_model_c/power_grid_model/include/power_grid_model/common/grouped_index_vector.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ class DenseGroupedIdxVector {

IdxVector const* dense_vector_{};
Idx group_{};
std::pair<group_iterator, group_iterator> group_range_{};
std::pair<group_iterator, group_iterator> group_range_;

friend class boost::iterator_core_access;

Expand Down
8 changes: 4 additions & 4 deletions ..._grid_model_c/power_grid_model/include/power_grid_model/common/iterator_like_concepts.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@ concept iterator_like = requires(T const t) {

template <typename T, typename ElementType>
concept forward_iterator_like = std::regular<T> && iterator_like<T, ElementType> && requires(T t) {
{ t++ } -> std::same_as<T>;
{ ++t } -> std::same_as<T&>;
{ t++ } -> std::same_as<T>; // NOLINT(bugprone-inc-dec-in-conditions)
{ ++t } -> std::same_as<T&>; // NOLINT(bugprone-inc-dec-in-conditions)
};

template <typename T, typename ElementType>
concept bidirectional_iterator_like = forward_iterator_like<T, ElementType> && requires(T t) {
{ t-- } -> std::same_as<T>;
{ --t } -> std::same_as<T&>;
{ t-- } -> std::same_as<T>; // NOLINT(bugprone-inc-dec-in-conditions)
{ --t } -> std::same_as<T&>; // NOLINT(bugprone-inc-dec-in-conditions)
};

template <typename T, typename ElementType>
Expand Down
7 changes: 4 additions & 3 deletions power_grid_model_c/power_grid_model/include/power_grid_model/common/three_phase_tensor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

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

this is very strange

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@TonyXiang8787 TonyXiang8787 Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}

// zero tensor
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

also this seems to be very strange.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

see #882 (comment)

}
} else {
for (size_t i = 0; i != 3; ++i) {
if (!is_nan(new_value(i))) {
current_value(i) = scalar * new_value(i);
current_value(i) = scalar * new_value(i); // can't forward due to runtime element access
}
(void)std::forward<Proxy>(current_value);
Comment on lines +359 to +361
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

see #882 (comment)

}
}
}
Expand Down
14 changes: 9 additions & 5 deletions power_grid_model_c/power_grid_model/include/power_grid_model/component/fault.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ class Fault final : public Base {
double r_f_;
double x_f_;

void check_sanity() {
void check_sanity() const {
using enum FaultPhase;

auto const check_supported = [&](auto const& iterable) {
Expand All @@ -203,15 +203,19 @@ class Fault final : public Base {
};
switch (fault_type_) {
case FaultType::three_phase:
return check_supported(std::array{FaultPhase::nan, default_value, abc});
check_supported(std::array{FaultPhase::nan, default_value, abc});
return;
case FaultType::single_phase_to_ground:
return check_supported(std::array{FaultPhase::nan, default_value, a, b, c});
check_supported(std::array{FaultPhase::nan, default_value, a, b, c});
return;
case FaultType::two_phase:
[[fallthrough]];
case FaultType::two_phase_to_ground:
return check_supported(std::array{FaultPhase::nan, default_value, ab, ac, bc});
check_supported(std::array{FaultPhase::nan, default_value, ab, ac, bc});
return;
case FaultType::nan:
return check_supported(std::array{FaultPhase::nan, default_value, abc, a, b, c, ab, ac, bc});
check_supported(std::array{FaultPhase::nan, default_value, abc, a, b, c, ab, ac, bc});
return;
default:
throw InvalidShortCircuitType(fault_type_);
}
Expand Down
4 changes: 2 additions & 2 deletions power_grid_model_c/power_grid_model/include/power_grid_model/component/source.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ class Source : public Appliance {
double u_ref_;
double u_ref_angle_;
// positive and zero sequence ref
DoubleComplex y1_ref_{};
DoubleComplex y0_ref_{};
DoubleComplex y1_ref_;
DoubleComplex y0_ref_;

template <symmetry_tag sym_calc> ApplianceSolverOutput<sym_calc> u2si(ComplexValue<sym_calc> const& u) const {
ApplianceSolverOutput<sym_calc> appliance_solver_output;
Expand Down
2 changes: 1 addition & 1 deletion power_grid_model_c/power_grid_model/include/power_grid_model/component/transformer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ class Transformer : public Branch {
y_series = (1.0 / z_series) / base_y_to;
// shunt
DoubleComplex y_shunt;
// Y = I0_2 / (U2/sqrt(3)) = i0 * (S / sqrt(3) / U2) / (U2/sqrt(3)) = i0 * S * / U2 / U2
// Y = I0_2 / (U2/sqrt3) = i0 * (S / sqrt3 / U2) / (U2/sqrt3) = i0 * S * / U2 / U2
double const y_shunt_abs = i0_ * sn_ / u2 / u2;
// G = P0 / (U2^2)
y_shunt.real(p0_ / u2 / u2);
Expand Down
2 changes: 1 addition & 1 deletion power_grid_model_c/power_grid_model/include/power_grid_model/main_core/output.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ template <typename Component, typename IndexType, class ComponentContainer, type
decltype(*comp_base_sequence_cbegin<Component>(MainModelState<ComponentContainer>{}))>
constexpr ResIt produce_output(MainModelState<ComponentContainer> const& state, ResIt res_it, ResFunc&& func) {
return std::transform(get_component_citer<Component>(state).begin(), get_component_citer<Component>(state).end(),
comp_base_sequence_cbegin<Component>(state), res_it, func);
comp_base_sequence_cbegin<Component>(state), res_it, std::forward<ResFunc>(func));
}

} // namespace detail
Expand Down
1 change: 1 addition & 0 deletions power_grid_model_c/power_grid_model/include/power_grid_model/main_core/update.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

why this?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Same here for #882 (comment).

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

}

template <typename T> bool check_id_na(T const& obj) {
Expand Down
2 changes: 2 additions & 0 deletions ...model_c/power_grid_model/include/power_grid_model/math_solver/common_solver_functions.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

why this?

Copy link
Member Author

@mgovers mgovers Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

template <symmetry_tag sym, typename LoadGenFunc>
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

why this?

Copy link
Member Author

@mgovers mgovers Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

See #882 (comment)

}

template <symmetry_tag sym>
Expand Down
26 changes: 13 additions & 13 deletions ...odel_c/power_grid_model/include/power_grid_model/math_solver/newton_raphson_se_solver.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,17 +93,17 @@ template <symmetry_tag sym_type> class NewtonRaphsonSESolver {
static constexpr auto is_iterative = true;

private:
enum class Order { row_major = 0, column_major = 1 };
enum class Order : IntS { row_major = 0, column_major = 1 };

struct NRSEVoltageState {
ComplexTensor<sym> ui_ui_conj{};
ComplexTensor<sym> uj_uj_conj{};
ComplexTensor<sym> ui_uj_conj{};
ComplexTensor<sym> uj_ui_conj{};
ComplexValue<sym> ui{};
ComplexValue<sym> uj{};
RealDiagonalTensor<sym> abs_ui_inv{};
RealDiagonalTensor<sym> abs_uj_inv{};
ComplexTensor<sym> ui_ui_conj;
ComplexTensor<sym> uj_uj_conj;
ComplexTensor<sym> ui_uj_conj;
ComplexTensor<sym> uj_ui_conj;
ComplexValue<sym> ui;
ComplexValue<sym> uj;
RealDiagonalTensor<sym> abs_ui_inv;
RealDiagonalTensor<sym> abs_uj_inv;

auto const& u_chi_u_chi_conj(Order ij_voltage_order) const {
return ij_voltage_order == Order::row_major ? ui_ui_conj : uj_uj_conj;
Expand All @@ -120,10 +120,10 @@ template <symmetry_tag sym_type> class NewtonRaphsonSESolver {
};

struct NRSEJacobian {
RealTensor<sym> dP_dt{};
RealTensor<sym> dP_dv{};
RealTensor<sym> dQ_dt{};
RealTensor<sym> dQ_dv{};
RealTensor<sym> dP_dt;
RealTensor<sym> dP_dv;
RealTensor<sym> dQ_dt;
RealTensor<sym> dQ_dv;

NRSEJacobian& operator+=(NRSEJacobian const& other) {
this->dP_dt += other.dP_dt;
Expand Down
1 change: 1 addition & 0 deletions ...r_grid_model_c/power_grid_model/include/power_grid_model/math_solver/sparse_lu_solver.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ template <rk2_tensor Matrix> class DenseLUFactor {
throw SparseMatrixError{}; // can not specify error code
}
}
(void)std::move(matrix);
Copy link
Member

Choose a reason for hiding this comment

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

Why here?

Copy link
Member Author

@mgovers mgovers Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

}
};

Expand Down
8 changes: 4 additions & 4 deletions power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/y_bus.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,12 +527,12 @@ template <symmetry_tag sym> class YBus {
std::shared_ptr<MathModelParam<sym> const> math_model_param_;

// cache the branch and shunt parameters in sequence_idx_map
IdxVector branch_param_idx_{};
IdxVector shunt_param_idx_{};
IdxVector branch_param_idx_;
IdxVector shunt_param_idx_;

// map index between admittance entries and parameter entries
std::vector<IdxVector> map_admittance_param_branch_{};
std::vector<IdxVector> map_admittance_param_shunt_{};
std::vector<IdxVector> map_admittance_param_branch_;
std::vector<IdxVector> map_admittance_param_shunt_;

std::unordered_map<uint64_t, ParamChangedCallback> parameters_changed_callbacks_;

Expand Down
12 changes: 6 additions & 6 deletions ...id_model_c/power_grid_model/include/power_grid_model/optimizer/tap_position_optimizer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ using TrafoGraphEdges = std::vector<std::pair<TrafoGraphIdx, TrafoGraphIdx>>;
using TrafoGraphEdgeProperties = std::vector<TrafoGraphEdge>;

struct RegulatedObjects {
std::set<Idx> transformers{};
std::set<Idx> transformers3w{};
std::set<Idx> transformers;
std::set<Idx> transformers3w;
};

using TransformerGraph = boost::compressed_sparse_row_graph<boost::directedS, TrafoGraphVertex, TrafoGraphEdge,
Expand Down Expand Up @@ -228,7 +228,7 @@ inline void process_edges_dijkstra(Idx v, std::vector<EdgeWeight>& vertex_distan
using TrafoGraphElement = std::pair<EdgeWeight, TrafoGraphIdx>;
std::priority_queue<TrafoGraphElement, std::vector<TrafoGraphElement>, std::greater<>> pq;
vertex_distances[v] = 0;
pq.push({0, v});
pq.emplace(0, v);

while (!pq.empty()) {
auto [dist, u] = pq.top();
Expand All @@ -246,10 +246,10 @@ inline void process_edges_dijkstra(Idx v, std::vector<EdgeWeight>& vertex_distan
// We can not use BGL_FORALL_OUTEDGES here because our grid is undirected
if (u == s && vertex_distances[s] + weight < vertex_distances[t]) {
vertex_distances[t] = vertex_distances[s] + weight;
pq.push({vertex_distances[t], t});
pq.emplace(vertex_distances[t], t);
} else if (u == t && vertex_distances[t] + weight < vertex_distances[s]) {
vertex_distances[s] = vertex_distances[t] + weight;
pq.push({vertex_distances[s], s});
pq.emplace(vertex_distances[s], s);
}
}
}
Expand Down Expand Up @@ -603,7 +603,7 @@ class TapPositionOptimizerImpl<std::tuple<TransformerTypes...>, StateCalculator,
using TransformerRanker = TransformerRanker_;

private:
std::vector<uint64_t> max_tap_ranges_per_rank{};
std::vector<uint64_t> max_tap_ranges_per_rank;
using ComponentContainer = typename State::ComponentContainer;
using RegulatedTransformer = TapRegulatorRef<TransformerTypes...>;
using UpdateBuffer = std::tuple<std::vector<typename TransformerTypes::UpdateType>...>;
Expand Down
3 changes: 2 additions & 1 deletion power_grid_model_c/power_grid_model/include/power_grid_model/sparse_ordering.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,8 @@ inline std::pair<IdxVector, std::vector<std::pair<Idx, Idx>>> minimum_degree_ord
alpha.push_back(alpha.back() == from ? to : from);
return {alpha, fills};
}
std::ranges::copy(detail::remove_vertices_update_degrees(u, d, dgd, fills), std::back_inserter(alpha));
auto result = detail::remove_vertices_update_degrees(u, d, dgd, fills);
alpha.insert(alpha.end(), result.begin(), result.end());
if (d.empty()) {
return {alpha, fills};
}
Expand Down
3 changes: 2 additions & 1 deletion power_grid_model_c/power_grid_model/include/power_grid_model/topology.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ class Topology {
permuted_node_indices[reordered[idx]] = n_non_cyclic_nodes + idx;
}

std::ranges::copy(reordered, std::back_inserter(dfs_node));
dfs_node.insert(dfs_node.end(), reordered.begin(), reordered.end());
for (auto [from, to] : fills) {
auto from_reordered = permuted_node_indices[from];
auto to_reordered = permuted_node_indices[to];
Expand Down Expand Up @@ -530,6 +530,7 @@ class Topology {
coupling[topo_comp_i] = Idx2D{topo_idx, new_math_comp_i};
}
}
(void)std::forward<GetMathTopoComponent>(get_component_topo);
Copy link
Member

Choose a reason for hiding this comment

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

why is this happening?

Copy link
Member Author

@mgovers mgovers Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

See #882 (comment).

The argument get_component_topo should be passed by value as lambdas.

}

void couple_all_appliance() {
Expand Down
Loading
Loading