cppcheck config - Githubissues

vchernin commented 2 years ago

Experimenting with cppcheck it seems this is a decent config:

cppcheck --enable=all --enable=information --inconclusive --error-exitcode=1 --suppress=missingIncludeSystem --suppress=unusedStructMember --suppress=unusedFunction --suppress=unknownMacro --inline-suppr --library=gtk --library=posix  ./src 2> err.txt

Unfortunately the upstream gtk config will need a lot of work so we can stop suppressing unusedStructMember, unusedFunction and unknownMacro, as those seem to result in lots of unhelpful warnings.

But the rest seem like potentially real code quality complaints.

cppcheck

```cpp [1msrc/application.cpp:83:99: [31mperformance:[39m Function parameter 'name' should be passed by const reference. [passedByValue][0m self->data->connections.push_back(self->pm->new_default_sink_name.connect([=](const std::string name) { ^ [1msrc/application.cpp:91:101: [31mperformance:[39m Function parameter 'name' should be passed by const reference. [passedByValue][0m self->data->connections.push_back(self->pm->new_default_source_name.connect([=](const std::string name) { ^ [1msrc/application.cpp:247:14: [31mstyle:[39m Consider using std::accumulate algorithm instead of a raw loop. [useStlAlgorithm][0m list += name + ","; ^ [1msrc/application.cpp:255:14: [31mstyle:[39m Consider using std::accumulate algorithm instead of a raw loop. [useStlAlgorithm][0m list += name + ","; ^ [1msrc/application_ui.cpp:166:78: [31mperformance:[39m Function parameter 'title' should be passed by const reference. [passedByValue][0m ->presets_manager->preset_load_error.connect([=](const std::string title, const std::string descr) { ^ [1msrc/application_ui.cpp:166:103: [31mperformance:[39m Function parameter 'descr' should be passed by const reference. [passedByValue][0m ->presets_manager->preset_load_error.connect([=](const std::string title, const std::string descr) { ^ [1msrc/compressor.cpp:222:21: [31mstyle:[39m Unused variable: serial [unusedVariable][0m for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/compressor_ui.cpp:136:21: [31mstyle:[39m Local variable 'serial' shadows outer variable [shadowVariable][0m for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/compressor_ui.cpp:122:8: [2mnote:[0m Shadowed declaration auto serial = get_new_filter_serial(); ^ [1msrc/compressor_ui.cpp:136:21: [2mnote:[0m Shadow variable for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/equalizer_ui.cpp:105:10: [31mstyle:[39m The scope of the variable 'freq1' can be reduced. [variableScope][0m double freq1 = 0.0; ^ [1msrc/equalizer_ui.cpp:105:16: [31mstyle:[39m Variable 'freq1' is assigned a value that is never used. [unreadVariable][0m double freq1 = 0.0; ^ [1msrc/gate.cpp:227:21: [31mstyle:[39m Unused variable: serial [unusedVariable][0m for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/gate_ui.cpp:121:21: [31mstyle:[39m Local variable 'serial' shadows outer variable [shadowVariable][0m for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/gate_ui.cpp:107:8: [2mnote:[0m Shadowed declaration auto serial = get_new_filter_serial(); ^ [1msrc/gate_ui.cpp:121:21: [2mnote:[0m Shadow variable for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/limiter.cpp:199:21: [31mstyle:[39m Unused variable: serial [unusedVariable][0m for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/limiter_ui.cpp:107:21: [31mstyle:[39m Local variable 'serial' shadows outer variable [shadowVariable][0m for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/limiter_ui.cpp:93:8: [2mnote:[0m Shadowed declaration auto serial = get_new_filter_serial(); ^ [1msrc/limiter_ui.cpp:107:21: [2mnote:[0m Shadow variable for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/lv2_wrapper.cpp:358:65: [31mstyle:[39m Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm][0m if (p.type == PortType::TYPE_CONTROL && p.symbol == symbol) { ^ [1msrc/multiband_compressor.cpp:190:21: [31mstyle:[39m Unused variable: serial [unusedVariable][0m for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/multiband_compressor_ui.cpp:157:21: [31mstyle:[39m Local variable 'serial' shadows outer variable [shadowVariable][0m for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/multiband_compressor_ui.cpp:139:8: [2mnote:[0m Shadowed declaration auto serial = get_new_filter_serial(); ^ [1msrc/multiband_compressor_ui.cpp:157:21: [2mnote:[0m Shadow variable for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/multiband_gate.cpp:199:21: [31mstyle:[39m Unused variable: serial [unusedVariable][0m for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/multiband_gate_ui.cpp:157:21: [31mstyle:[39m Local variable 'serial' shadows outer variable [shadowVariable][0m for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/multiband_gate_ui.cpp:139:8: [2mnote:[0m Shadowed declaration auto serial = get_new_filter_serial(); ^ [1msrc/multiband_gate_ui.cpp:157:21: [2mnote:[0m Shadow variable for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/pipe_manager.cpp:270:24: [31mwarning:[39m Identical condition 'PipeManager::exiting', second condition is always false [identicalConditionAfterEarlyExit][0m if (PipeManager::exiting) { ^ [1msrc/pipe_manager.cpp:219:18: [2mnote:[0m If condition 'PipeManager::exiting' is true, the function will return/exit if (PipeManager::exiting) { ^ [1msrc/pipe_manager.cpp:270:24: [2mnote:[0m Testing identical condition 'PipeManager::exiting' if (PipeManager::exiting) { ^ [1msrc/pipe_manager.cpp:280:24: [31mwarning:[39m Identical condition 'PipeManager::exiting', second condition is always false [identicalConditionAfterEarlyExit][0m if (PipeManager::exiting) { ^ [1msrc/pipe_manager.cpp:219:18: [2mnote:[0m If condition 'PipeManager::exiting' is true, the function will return/exit if (PipeManager::exiting) { ^ [1msrc/pipe_manager.cpp:280:24: [2mnote:[0m Testing identical condition 'PipeManager::exiting' if (PipeManager::exiting) { ^ [1msrc/pipe_manager.cpp:290:24: [31mwarning:[39m Identical condition 'PipeManager::exiting', second condition is always false [identicalConditionAfterEarlyExit][0m if (PipeManager::exiting) { ^ [1msrc/pipe_manager.cpp:219:18: [2mnote:[0m If condition 'PipeManager::exiting' is true, the function will return/exit if (PipeManager::exiting) { ^ [1msrc/pipe_manager.cpp:290:24: [2mnote:[0m Testing identical condition 'PipeManager::exiting' if (PipeManager::exiting) { ^ [1msrc/pipe_manager.cpp:302:24: [31mwarning:[39m Identical condition 'PipeManager::exiting', second condition is always false [identicalConditionAfterEarlyExit][0m if (PipeManager::exiting) { ^ [1msrc/pipe_manager.cpp:219:18: [2mnote:[0m If condition 'PipeManager::exiting' is true, the function will return/exit if (PipeManager::exiting) { ^ [1msrc/pipe_manager.cpp:302:24: [2mnote:[0m Testing identical condition 'PipeManager::exiting' if (PipeManager::exiting) { ^ [1msrc/pipe_manager.cpp:490:13: [31m[35mwarning: inconclusive:[39m Possible null pointer dereference: pod_prop [nullPointer][0m switch (pod_prop->key) { ^ [1msrc/pipe_manager.cpp:482:28: [2mnote:[0m Assignment 'pod_prop=nullptr', assigned value is 0 spa_pod_prop* pod_prop = nullptr; ^ [1msrc/pipe_manager.cpp:490:13: [2mnote:[0m Null pointer dereference switch (pod_prop->key) { ^ [1msrc/pipe_manager.cpp:712:32: [31mstyle:[39m Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm][0m if (module.id == info->id) { ^ [1msrc/pipe_manager.cpp:738:32: [31mstyle:[39m Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm][0m if (client.id == info->id) { ^ [1msrc/pipe_manager.cpp:1561:79: [31mstyle:[39m Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm][0m if (link.output_node_id == id && link.input_node_id == ee_sink_node.id) { ^ [1msrc/pipe_manager.cpp:1567:81: [31mstyle:[39m Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm][0m if (link.output_node_id == ee_source_node.id && link.input_node_id == id) { ^ [1msrc/pipe_manager.cpp:1645:7: [31mstyle:[39m Consider using std::count_if algorithm instead of a raw loop. [useStlAlgorithm][0m count++; ^ [1msrc/pipe_manager.cpp:1023:33: [31mwarning:[39m String literal "ee_" doesn't match length argument for substr(). [incorrectStringCompare][0m if (description.substr(0, 2) == "ee_") { ^ [1msrc/pipe_manager.cpp:1496:23: [31mstyle:[39m Unused variable: serial [unusedVariable][0m for (const auto& [serial, node] : node_map) { ^ [1msrc/pipe_manager.cpp:1549:15: [31mstyle:[39m Unused variable: serial [unusedVariable][0m for (auto& [serial, node] : node_map) { ^ [1msrc/pipe_manager_box.cpp:207:12: [31mstyle:[39m Consider using std::transform algorithm instead of a raw loop. [useStlAlgorithm][0m values.push_back(ui::holders::create(info)); ^ [1msrc/pipe_manager_box.cpp:222:12: [31mstyle:[39m Consider using std::transform algorithm instead of a raw loop. [useStlAlgorithm][0m values.push_back(ui::holders::create(info)); ^ [1msrc/plugins_menu.cpp:28:3: [31mwarning:[39m Member variable 'Data::schedule_signal_idle' is not initialized in the constructor. [uninitMemberVar][0m Data() { this->translated = tags::plugin_name::get_translated(); } ^ [1msrc/plugins_menu.cpp:28:3: [31mwarning:[39m Member variable 'Data::application' is not initialized in the constructor. [uninitMemberVar][0m Data() { this->translated = tags::plugin_name::get_translated(); } ^ [1msrc/presets_manager.cpp:257:20: [31mstyle:[39m Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm][0m if (p == name) { ^ [1msrc/presets_manager.cpp:465:27: [31mstyle:[39m Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm][0m if (v == p) { ^ [1msrc/presets_manager.cpp:517:27: [31mstyle:[39m Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm][0m if (v == p) { ^ [1msrc/presets_manager.cpp:295:28: [31mstyle:[39m Unused variable: blocklist [unusedVariable][0m std::vector blocklist; ^ [1msrc/rnnoise.cpp:42:0: [31merror:[39m failed to expand 'g_signal_connect', it is invalid to use a preprocessor directive as macro parameter [preprocessorErrorDirective][0m #ifdef RNNOISE_AVAILABLE ^ [1msrc/rnnoise.cpp:0:0: [31minformation:[39m This file is not analyzed. Cppcheck failed to extract a valid configuration. Use -v for more details. [noValidConfiguration][0m ^ [1msrc/stream_input_effects.cpp:193:21: [31mstyle:[39m Unused variable: serial [unusedVariable][0m for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/stream_output_effects.cpp:189:21: [31mstyle:[39m Unused variable: serial [unusedVariable][0m for (const auto& [serial, node] : pm->node_map) { ^ [1msrc/util.cpp:217:12: [31mstyle:[39m Consider using std::transform algorithm instead of a raw loop. [useStlAlgorithm][0m output.push_back(v.c_str()); ^ [1mnofile:0:0: [31minformation:[39m Cppcheck cannot find all the include files (use --check-config for details) [missingInclude][0m ```

wwmm commented 2 years ago

When writing C/C++ code I use clang and clang-tidy. As cppcheck is based on a very different approach it may catch some things the other linters can't and vice versa. But just like the other linters I see it also gives some useless suggestions we will need to suppress:

Unused variable: serial. In this case I do not think there is something better that can be done. It is the syntax of the structured binding. It just happens we do not need all the variables.
If condition 'PipeManager::exiting' is true, the function will return/exit. That is exactly the reason why the check is there :smile: .
Some of the std::find_if are questionable and do not seem to be really better than a raw loop.
We have to be careful with suggestion that require pass by reference. In some places we should not do this. But I think that in the locations suggested by cppcheck it would be fine to do it.

That being said it caught some unused variables that for some reason clang is not warning me about in vscode. I think the reason is that as clang is not recognizing the source_location namespace it stopped showing me some warnings. Implementing a github action with cppcheck is a good idea even if we can't suppress the bad information.

wwmm commented 2 years ago

The scope of the variable 'freq1' can be reduced. Variable 'freq1' is assigned a value that is never used.

I agree with cppcheck on this too.

vchernin commented 2 years ago

Yes, I see the same source_location problem with clang-tidy here. What is weird is meson includes a clang-tidy target (which is very useful for CI) but it seems to repeat complaints about source-location far more than it should. Maybe it is because util where source-location is, is imported all over the codebase?

The current config also gives a lot of:

easyeffects/include/filter_ui.hpp:33:1: warning: parameter name '_l' is too short, expected at least 3 characters [readability-identifier-length]
G_DECLARE_FINAL_TYPE(FilterBox, filter_box, EE, FILTER_BOX, GtkBox)

Also I could originally not run clang-tidy at all with meson as it gave parsing errors, because there was a typo in .clang-tidy:

diff --git a/.clang-tidy b/.clang-tidy
index f6a562d8..4dd82143 100644
--- a/.clang-tidy
+++ b/.clang-tidy
@@ -2,4 +2,4 @@ Checks: 'boost-*,bugprone-*,cert-*,cppcoreguidelines-*,modernize-*,performance-*
 -clang-analyzer-osx*,-cppcoreguidelines-pro-type-cstyle-cast,-cppcoreguidelines-pro-type-vararg,
 -cppcoreguidelines-non-private-member-variables-in-classes,-cppcoreguidelines-avoid-magic-numbers,
 -readability-magic-numbers,-cppcoreguidelines-owning-memory,
--readability-function-cognitive-complexity',-bugprone-easily-swappable-parameters
+-readability-function-cognitive-complexity,-bugprone-easily-swappable-parameters'

How is it that vscode is still working? I have not tried the extension myself.

wwmm commented 2 years ago

Maybe it is because util where source-location is, is imported all over the codebase?

I don't think so. The problem is clang does not support c++20 source_location yet. I think EasyEffects probably does not compile on clang at this moment.

Also I could originally not run clang-tidy at all with meson as it gave parsing errors, because there was a typo in .clang-tidy: How is it that vscode is still working? I have not tried the extension myself.

Oh! I did not even notice this happened. Thanks! Somehow the vscode extension did not say anything about it. At least not in a place I use to check. Now that I fixed this problem clang-tidy is showing lots of things it was not showing before. I will update the master branch.

wwmm commented 2 years ago

Even after fixing the bug in our clang-tidy configuration it still does not see the unused std::vector<std::string> blocklist variable that cppcheck caught. Maybe because the source_location warn is still here as clang does not understand it. In any case I think that having an action for clang-tidy and another for cppcheck is probably the best thing to do. As time goes by we add some suppressions like I already had to do with clang-tidy.

vchernin commented 2 years ago

I seem to have made clang happy by switching std::source_location to std::experimental::source_location, and including experimental/source_location. Now the clang-tidy warnings make sense.

There doesn't seem to be a way to suppress clang build errors from clang-tidy without actually fixing them, so maybe it is worth ifdefing just for the sake of reducing clang-tidy output? (and I guess improving compatibility couldn't hurt). It still seems to build normally with gcc with std::experimental, but I think it is better to ifdef than to include things from experimental for gcc builds.

vchernin commented 2 years ago

it still seems to build normally with gcc with std::experimental

Nevermind, this causes linking failure.

wwmm commented 2 years ago

so maybe it is worth ifdefing just for the sake of reducing clang-tidy output? (and I guess improving compatibility couldn't hurt) but I think it is better to ifdef than to include things from experimental for gcc builds.

Yes. As gcc builds just fine it is probably not a good idea to have only an experimental header. The question is how to do this #ifdef. We will probably need to improve our Meson scripts in a way we check which compiler is being used so we can do something similar to the one done to the RNNoise plugin.

wwmm commented 2 years ago

We will probably need to improve our Meson scripts in a way we check which compiler is being used

But if we do this and one day clang decides to not use the experimental header anymore because the feature is stable compilation will be broken with it again. We have to check the compiler and its version.

vchernin commented 2 years ago

We have to check the compiler and its version.

I think it is alright since if the configuration is not correct, clang-tidy should fail in CI. I am not sure how much knowing the version would help since we do not know when source_location will become stable, so we would have to keep adding hardcoded versions where source_location is not stable.

Also I didn't need to touch meson, to fix this I just used __clang__:

diff --git a/include/util.hpp b/include/util.hpp
index 36d9b655..3c63bf79 100644
--- a/include/util.hpp
+++ b/include/util.hpp
@@ -34,6 +34,9 @@
 #include <string>
 #include <thread>
 #include <vector>
+#ifdef __clang__
+#include <experimental/source_location>
+#endif

 namespace util {

@@ -44,11 +47,23 @@ constexpr double minimum_db_d_level = -100.0;
 constexpr float minimum_linear_level = 0.00001F;
 constexpr double minimum_linear_d_level = 0.00001;

+#ifndef __clang__
 void debug(const std::string& s, std::source_location location = std::source_location::current());
 void error(const std::string& s, std::source_location location = std::source_location::current());
 void critical(const std::string& s, std::source_location location = std::source_location::current());
 void warning(const std::string& s, std::source_location location = std::source_location::current());
 void info(const std::string& s, std::source_location location = std::source_location::current());
+#endif
+
+
+// otherwise clang-tidy complains about: error: no type named 'source_location' in namespace 'util' [clang-diagnostic-error] which is a clang build error due to unfinished support for c++20
+#ifdef __clang__
+void debug(const std::string& s, std::experimental::source_location location = std::experimental::source_location::current());
+void error(const std::string& s, std::experimental::source_location location = std::experimental::source_location::current());
+void critical(const std::string& s, std::experimental::source_location location = std::experimental::source_location::current());
+void warning(const std::string& s, std::experimental::source_location location = std::experimental::source_location::current());
+void info(const std::string& s, std::experimental::source_location location = std::experimental::source_location::current());
+#endif

 auto normalize(const double& x, const double& max, const double& min = 1.0) -> double;

a bit duplicated but it at least seems to be working correctly.

wwmm commented 2 years ago

I am not sure how much knowing the version would help since we do not know when source_location will become stable, so we would have to keep adding hardcoded versions where source_location is not stable.

At this moment yes. But we would essentially just be telling if clang <= version use the experimental header.

Also I didn't need to touch meson, to fix this I just used clang:

What will make the compilation fail once clang has stable support for source_location because this header check applies to all clang versions. But I doubt a pretty solution will be available right now.

vchernin commented 2 years ago

But we would essentially just be telling if clang <= version use the experimental header.

Personally, once clang improves their support I don't think we should continue to support older clang versions that require std::experimental. Seeing as there are only gcc builds currently, I doubt there are many in the future who will want to build with clang but insist on some older version. If they really want to they'd be better off using a patch.

So indeed since there are no pretty solutions I think just waiting for an inevitable CI compilation failure is best.

vchernin commented 2 years ago

I realized in #1739 I missed std:source_location is also in util.cpp... so while this fixes most of the errors there is no point merging a half done fix. I will try figure out the action later.

wwmm / easyeffects

cppcheck config #1738