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 all 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
12 changes: 6 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 @@ -76,10 +76,10 @@ struct MapArray {
struct JsonConverter : msgpack::null_visitor {
static constexpr char sep_char = ' ';

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

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,7 +606,7 @@ class Serializer {
});
}
void pack_attribute(AttributeBuffer<void const> const& attribute_buffer, Idx idx) {
return ctype_func_selector(attribute_buffer.meta_attribute->ctype, [&]<class T> {
ctype_func_selector(attribute_buffer.meta_attribute->ctype, [this, &attribute_buffer, idx]<class T> {
packer_.pack(*(reinterpret_cast<T const*>(attribute_buffer.data) + idx));
});
}
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
2 changes: 1 addition & 1 deletion power_grid_model_c/power_grid_model/include/power_grid_model/main_core/topology.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ constexpr void register_topo_components(MainModelState<ComponentContainer> const
auto const end = get_component_citer<Component>(state).end();

target.resize(std::distance(begin, end));
std::transform(begin, end, target.begin(), func);
std::transform(begin, end, target.begin(), 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
24 changes: 0 additions & 24 deletions power_grid_model_c/power_grid_model/include/power_grid_model/main_model.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,34 +55,10 @@ class MainModel {
impl().get_indexer(component_type, id_begin, size, indexer_begin);
}

void set_construction_complete() { impl().set_construction_complete(); }

template <class CompType> void add_component(std::vector<typename CompType::InputType> const& components) {
add_component<CompType>(std::span<typename CompType::InputType const>{components});
}
template <class CompType> void add_component(std::span<typename CompType::InputType const> components) {
impl().add_component<CompType>(components);
}

template <cache_type_c CacheType> void update_components(ConstDataset const& update_data) {
impl().update_components<CacheType>(update_data.get_individual_scenario(0));
}

template <typename CompType, typename MathOutputType, typename OutputType>
void output_result(MathOutputType const& math_output, std::vector<OutputType>& target) const {
output_result<CompType>(math_output, std::span<OutputType>{target});
}
template <typename CompType, typename MathOutputType, typename OutputType>
void output_result(MathOutputType const& math_output, std::span<OutputType> target) const {
impl().output_result<CompType>(math_output, target.begin());
}

template <calculation_type_tag calculation_type, symmetry_tag sym> auto calculate(Options const& options) {
return impl().calculate<calculation_type, sym>(options);
}
void calculate(Options const& options, MutableDataset const& result_data) {
return impl().calculate(options, result_data);
}
BatchParameter calculate(Options const& options, MutableDataset const& result_data,
ConstDataset const& update_data) {
return impl().calculate(options, result_data, update_data);
Expand Down
Loading
Loading