botan icon indicating copy to clipboard operation
botan copied to clipboard

Reduce/Fix cppcheck warnings in utils directory

Open KaganCanSit opened this issue 6 months ago • 2 comments

This PR addresses multiple cppcheck warnings found in the utils directory to improve code quality and performance.

Changes Made:

  • [botan/src/lib/utils/read_kv.cpp:19] (style) The scope of the variable 'parts' can be reduced. Warning: Be careful when fixing this message, especially when there are inner loops.

Note: split_on seems to be called here for a check. The parts vector is already not used. Perhaps it might be healthier to go for one of the following solutions; - Remove the parts vector completely and call only split_on. - Evaluate the return as an explicit intent to emphasize that split_on is called for explicit control.

static_cast<void>(split_on(kv, ‘i’));
  • [botan/src/lib/utils/read_kv.cpp:59] (style) The statement 'if (escaped) escaped=false' is logically equivalent to 'escaped=false'. [duplicateConditionalAssign]

  • [botan/src/lib/utils/os_utils/os_utils.cpp:805] (performance) When an object of a class is created, the constructors of all member variables are called consecutively in the order the variables are declared, even if you don't explicitly write them to the initialization list. You could avoid assigning 'm_input_handle' a value by passing the value to the constructor in the initialization list. [useInitializationList]

Note: GetStdHandle does not throw an exception because it is a C API function.

  • [botan/src/lib/utils/parsing.cpp:432] (style) Condition 'i>0' is always true [knownConditionTrueFalse]

Note: If i > 0, then name[i-1] is always accessible, so no bounds check needed

  • [botan/src/lib/utils/http_util/http_util.cpp:146] (performance) Ineffective call of function 'substr' because a prefix of the string is assigned to itself. Use resize() or pop_back() instead. [uselessCallsSubstr]

Last Status

No functional changes, only optimization and cleaning improvements.

Passes all available unit tests. Compiled and tested with the following command call.

ninja clean && ./configure.py --without-documentation --cc=clang --compiler-cache=ccache --build-targets=static,cli,tests --build-tool=ninja && ninja && ./botan-test --test-threads=4 --run-long-tests

If want me to make changes due to the notes I left or for the arrangements made, it will be enough to leave a comment. I will try to deal with it as soon as possible.

Best regards.

KaganCanSit avatar Jul 15 '25 19:07 KaganCanSit

Coverage Status

coverage: 90.625% (+0.005%) from 90.62% when pulling 575a94d7e2052416e408374563a4b0c667b2ff47 on KaganCanSit:reduce-fix-cppcheck-utils-directory into 2d01ad0cedd1d5ae6c00ce372942df5c5a029e8a on randombit:master.

coveralls avatar Jul 15 '25 20:07 coveralls

  • ~01.11.25 - Refactored hostname, location, and service variables in http_util.cpp to be const by applying IIFE pattern. Rebased with master branch.~
  • ~02.11.25 - The line return std::tuple(std::move(host), std::move(loc), std::move(svc)); has been updated to avoid using std::move(), taking into account NRVO (Named Return Value Optimization) and guaranteed copy elision.~
  • 09.11.25 - Rebased with master branch.
  • 13.11.25 - Rebased with master branch. I decided that reverting IIFE usage wasn't worth the decrease in readability.
  • 16.11.25 - Rebased with master branch because CI error(/var/lib/man-db/auto-update).

KaganCanSit avatar Oct 31 '25 21:10 KaganCanSit