2nd round of cppcheck warning fixes #5125

seanm · 2025-01-07T19:17:09Z

No description provided.

seanm · 2025-01-07T19:18:01Z

@N-Dekker may interest you...

There are lots more of the 'use const references' warnings, but in public API, not sure if that's small enough a change to just do? It won't affect existing callers after all...

Modules/Filtering/ImageIntensity/include/itkMaskNegatedImageFilter.h

dzenanz · 2025-01-07T19:36:54Z

Modules/IO/XML/src/itkDOMNode.cxx

@@ -569,7 +569,7 @@ DOMNode::Find(const std::string & path)
    if (pos != std::string::npos)
    {
      const std::string s2 = s.substr(pos + 1);
-      s = s.substr(0, pos);
+      s.resize(pos);


Cool refactoring!

dzenanz

LGTM

N-Dekker · 2025-01-07T19:59:01Z

Modules/Core/Common/test/itkFixedArrayTest.cxx

-Print_Array(itk::FixedArray<int, 3> x, std::ostream & os)
+Print_Array(const itk::FixedArray<int, 3> & x, std::ostream & os)


In this particular case, either way is fine by me. itk::FixedArray<int, 3> can be copied very quickly, as it is trivially copy-able. So the original pass-by-value does not involve dynamic memory allocation or the execution of a hand-written copy-constructor.

In general, pass-by-value (rather than pass-by-const-reference) has the advantage that it is more thread-safe. With pass-by-const-reference, the caller is responsible for keeping the object alive and unmodified, while the function is running. So pass-by-value is often simpler.

However, when copying is more expensive (as with an arbitrary std::vector), it's often beneficial to go for pass-by-const-reference. Then again, sometimes move-semantics is even better...

So as always, each approach has its pro's and con's 🤔

Once we would have cppcheck on the CI, would it still be possible to allow pass-by-value (rather than pass-by-const-reference) in specific cases? For object types that can be copied very quickly. Or for specific functions?

Because as I wrote before, there are cases where pass-by-value is preferable.

I don't think it warns about every single pass-by-value under the sun, only for some classes where it usually makes sense, like std::string.

Indeed I see now that cppcheck does not warn for every pass-by-value (under the 🌞). Looks like it does not warn when you pass an empty struct by value. However, it does warn for itk::FixedArray. Looks like it even warns for very small ones, for example itk::FixedArray<int, 1> and itk::FixedArray<char, 1>. While passing those two by-value is probably faster than passing them by reference!

I'm not opposed to this specific code change, it's just a test anyway. But I just doubt if we should always enforce pass-by-const-reference whenever cppcheck tells us to do so.

FYI, I just posted at https://sourceforge.net/p/cppcheck/discussion/general/thread/cce6de4bfd/ "How to deal with passByValue (CWE: 398) warnings?"

N-Dekker · 2025-01-07T20:18:20Z

Modules/Core/Common/test/itkMathRoundTest.cxx

-math_test_helper(std::string str, bool test)
+math_test_helper(const std::string & str, bool test)


In this particular case, I would not expect any performance gain from using const std::string &, because math_test_helper is only called with a "literal" as argument. So if it's just about performance, I would use either const char * or (preferably) std::string_view as parameter type.

I'm interested to know if cppcheck allows passing std::string_view by value, because it's generally suggested to pass std::string_view by value.

cppcheck categorizes this under "performance", but this is just test code, so performance isn't an issue anyway.

OK, so this proposed change is only meant to make cppcheck happy, right?

This is cool: Allow passing std::string_view by value danmar/cppcheck#3817

So yes, cppcheck allows passing std::string_view by value 😃

OK, so this proposed change is only meant to make cppcheck happy, right?

Yes. :)

I just checked: replacing std::string with std::string_view does indeed make cppcheck happy, in this case :)

By the way, I just ran the Cppcheck 2.16.0 GUI application on Windows, analyzing source directory "ITK\Modules\Core\Common". I wasn't able to enable ITK_CPPCHECK_TEST in CMake. During CMake configuration, it says:

CMake Error at CMake/CppcheckTargets.cmake:131 (add_custom_command): TARGET 'all_cppcheck' was not created in this directory. Call Stack (most recent call first): CMake/ITKModuleCPPCheckTest.cmake:12 (add_cppcheck_dir) CMake/ITKModuleMacros.cmake:201 (itk_module_cppcheck_test) Modules/Core/Common/CMakeLists.txt:141 (itk_module_impl)

Does the CMake option ITK_CPPCHECK_TEST work for you?

I didn't use ITK_CPPCHECK_TEST. I used CMAKE_EXPORT_COMPILE_COMMANDS=ON then invoked with:

cppcheck --project=compile_commands.json --platform=unspecified --enable=style -q --library=qt --library=posix --library=gnu --library=bsd --library=windows --check-level=exhaustive --suppressions-list=../ITK/ITKcppcheckSuppressions.txt --template='{id},{file}:{line},{severity},{message}' -j22

I just checked: replacing std::string with std::string_view does indeed make cppcheck happy, in this case :)

OK, changed.

N-Dekker

Approved! The pass-by-const-references introduced with this PR are OK to me, it's just that I feel that cppcheck is too "pushy" with these passByValue warnings.

I would have preferred a few more string_view, rather than const std::string &, but I leave it up to you.

👍

N-Dekker · 2025-01-08T20:25:33Z

Modules/Core/Mesh/test/itkCellInterfaceTest.cxx

-TestCellInterface(std::string name, TCell * aCell)
+TestCellInterface(const std::string & name, TCell * aCell)


Of course, the name parameter type of this TestCellInterface could also be string_view.

N-Dekker · 2025-01-08T20:27:46Z

Modules/Core/QuadEdgeMesh/test/itkQuadEdgeMeshCellInterfaceTest.cxx

-TestCellInterface(std::string name, TCell * aCell)
+TestCellInterface(const std::string & name, TCell * aCell)


...and the name parameter of this TestCellInterface may also be string_view. (I guess it should also be placed in a different namespace, to avoid name conflicts with the other TestCellInterface function template, but apparently it "just" worked. That's probably beyond the scope of this PR anyway.)

N-Dekker · 2025-01-08T20:51:08Z

Modules/Core/QuadEdgeMesh/test/itkQuadEdgeMeshCellInterfaceTest.cxx

-TestQECellInterface(std::string name, TCell * aCell)
+TestQECellInterface(const std::string & name, TCell * aCell)


string_view

cppcheck identified this pattern, and suggested a fast alternative. Mutate the string instead of creating a copy.

cppcheck suggested all these. Happily does not affect callers of such functions. In some cases used std::string_view instead. I only changed non-public API here, for compatibility reasons. The warning is of the form: passedByValue,ITK/Modules/IO/PhilipsREC/src/itkPhilipsRECImageIO.cxx:211,performance,Function parameter 'imageTypesScanSequenceIndex' should be passed by const reference.

dzenanz · 2025-01-09T16:29:51Z

/azp run ITK.macOS

dzenanz reviewed Jan 7, 2025

View reviewed changes

Modules/Filtering/ImageIntensity/include/itkMaskNegatedImageFilter.h Outdated Show resolved Hide resolved

dzenanz reviewed Jan 7, 2025

View reviewed changes

seanm force-pushed the cppcheck2.16.0-part2 branch from 132040d to 49a2284 Compare January 7, 2025 19:51

dzenanz approved these changes Jan 7, 2025

View reviewed changes

N-Dekker reviewed Jan 7, 2025

View reviewed changes

seanm force-pushed the cppcheck2.16.0-part2 branch from 49a2284 to f95ea0c Compare January 8, 2025 17:25

N-Dekker approved these changes Jan 8, 2025

View reviewed changes

seanm added 3 commits January 8, 2025 17:30

ENH: fixed cppcheck uselessCallsSubstr warning, avoid string copy

fd9abd4

cppcheck identified this pattern, and suggested a fast alternative. Mutate the string instead of creating a copy.

COMP: removed unused private class member variable

f98848e

seanm force-pushed the cppcheck2.16.0-part2 branch from f95ea0c to 1ef2d1c Compare January 8, 2025 22:32

dzenanz merged commit 9ba9e28 into InsightSoftwareConsortium:master Jan 9, 2025
15 checks passed

dzenanz mentioned this pull request Jan 9, 2025

3rd round of cppcheck warning fixes #5127

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2nd round of cppcheck warning fixes #5125

2nd round of cppcheck warning fixes #5125

seanm commented Jan 7, 2025

seanm commented Jan 7, 2025

dzenanz Jan 7, 2025

dzenanz left a comment

N-Dekker Jan 7, 2025

N-Dekker Jan 7, 2025

seanm Jan 7, 2025

N-Dekker Jan 8, 2025

N-Dekker Jan 8, 2025

N-Dekker Jan 7, 2025 •

edited

Loading

seanm Jan 7, 2025

N-Dekker Jan 7, 2025

N-Dekker Jan 7, 2025

seanm Jan 7, 2025

N-Dekker Jan 8, 2025

seanm Jan 8, 2025

seanm Jan 8, 2025

N-Dekker left a comment

N-Dekker Jan 8, 2025

seanm Jan 8, 2025

N-Dekker Jan 8, 2025

seanm Jan 8, 2025

N-Dekker Jan 8, 2025

seanm Jan 8, 2025

dzenanz commented Jan 9, 2025

		Print_Array(itk::FixedArray<int, 3> x, std::ostream & os)
		Print_Array(const itk::FixedArray<int, 3> & x, std::ostream & os)

		math_test_helper(std::string str, bool test)
		math_test_helper(const std::string & str, bool test)

		TestCellInterface(std::string name, TCell * aCell)
		TestCellInterface(const std::string & name, TCell * aCell)

		TestQECellInterface(std::string name, TCell * aCell)
		TestQECellInterface(const std::string & name, TCell * aCell)

2nd round of cppcheck warning fixes #5125

2nd round of cppcheck warning fixes #5125

Conversation

seanm commented Jan 7, 2025

seanm commented Jan 7, 2025

Choose a reason for hiding this comment

dzenanz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Dekker Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Dekker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzenanz commented Jan 9, 2025

N-Dekker Jan 7, 2025 •

edited

Loading