Topic: Use std::algorithm when possible?
matthiakl Topic Opener |
Posted at: 2020-11-07, 14:01
cppcheck reports a lot issues of the type "Consider using std::count_if algorithm instead of a raw loop." but I am not sure, whether we want this. So for example
would become something like this:
Do we want to use this style, or leave it as it is? Top Quote |
GunChleoc |
Posted at: 2020-11-07, 15:39
I think the recommended style creates a wall of text. I find the old style more readable, even if it has a few more lines in it. Would it work like this?
Then I'd be OK with using it. Busy indexing nil values Top Quote |
matthiakl Topic Opener |
Posted at: 2020-11-07, 18:24
Nope:
I agree with you, that the current style is more readable. Lambdas are fancy, but sometimes hard to read / understand. Top Quote |
GunChleoc |
Posted at: 2020-11-07, 18:55
I'm in favor of leaving them as they are then. While we're talking about cppcheck, I'm wondering whether we should add a GitHub action for it too, with a list of checks yet to be cleared. Just like we did for clang-tidy. Busy indexing nil values Top Quote |
matthiakl Topic Opener |
Posted at: 2020-11-07, 20:23
I don't think a GitHub action is a good idea. Creating such a report takes a long time (~30min). Additionally it reports false positives (see examples below) or hints only, which would be difficult to handle in an automatic workflow. To use it simply to detect There is also a list of unused functions, but hesitated removing them straight away, because they might be intended for future use. Some examples: src/io/machdep.h:62: (error) Uninitialized variable: s => it is iniatialized src/economy/ship_fleet.cc:212: (warning) Non-pure function: 'get_schedule' is called inside assert statement. Assert statements are removed from release builds so the code inside assert statement is not executed. If the code is needed also in release builds, this is a bug. => can be a bug, or was intended src/logic/map_objects/tribes/requirements.h:82: (style) Struct 'Requirements' has a constructor with 1 argument that is not explicit. Such constructors should in general be explicit for type safety reasons. Using the explicit keyword in the constructor means some mistakes when using the class can be avoided. => explicit and templates don’t work together Top Quote |
GunChleoc |
Posted at: 2020-11-10, 07:42
Thanks for the analysis of the tool. Let's just stick with clang-tidy for the GitHub actions then. There's also a check for explicit constructors in there and it does recognize the templates, and we can get rid of false positives with a Busy indexing nil values Top Quote |