Wmissing variable declaration #4676

andrei-sandor · 2024-05-16T19:52:37Z

COMP: Fix missing declarations of unused variables.

This is to fix the -Wmissing-variable-declarations warning of clang.

This is simply related that the variable in question is not used in the file

COMP: Fix missing variables declaration (static)

This is a fix about -Wmissing-variable-declarations warning of clang

This is about the static case. A lot of variables were marked as warning due to non declaration. The static keyword was added when it makes sense that the variable is used internally.

Examples/RegistrationITKv4/DeformableRegistration17.cxx

dzenanz

You can apply Niels' suggestion if you wish, but the proper formatting has to be followed to satisfy the CI.

seanm · 2024-05-17T18:24:20Z

...proper formatting has to be followed to satisfy the CI.

Yeah, we haven't found a way to install old clang-format version 8, so we keep skipping the commit hooks. And newer clang-format makes unrelated changes. There's no magic "Do: reformat" command like with VTK's gitlab?

dzenanz · 2024-05-17T18:32:46Z

Normally, ITK builds the correct version of clang-format, and puts it e.g. here: C:\Dev\ITK-vs22\ClangFormat-prefix\src\ClangFormat\clang-format.exe. You should be able to copy that executable into a more suitable location, if you like. But also pre-commit hooks should be using it?

seanm · 2024-05-17T19:11:36Z

Normally, ITK builds the correct version of clang-format, and puts it e.g. here: C:\Dev\ITK-vs22\ClangFormat-prefix\src\ClangFormat\clang-format.exe. You should be able to copy that executable into a more suitable location, if you like. But also pre-commit hooks should be using it?

I have ITK_USE_CLANG_FORMAT=ON but I don't find any such thing built. That gets built in the bin directory?, or do you have to make install?

sean@hobgoblin ITK-bin % find . -iname "*clang*"
./KWStyle/Utilities/boost/config/compiler/clang_version.hpp
./KWStyle/Utilities/boost/config/compiler/clang.hpp
./KWStyle/Utilities/boost/predef/compiler/clang.h
./ITKClangFormatConfig.cmake

dzenanz · 2024-05-17T19:59:13Z

Here is the output on my Linux build:

dzenan@corista:~/ITK-git-rel$ find . -iname "clang*"
./temp/clang-format-Linux
./Wrapping/Generators/CastXML/castxml/share/castxml/clang
./Wrapping/Generators/CastXML/castxml-prefix/src/castxml/share/castxml/clang
./clang-format-Linux
./ClangFormat-prefix
./ClangFormat-prefix/src/ClangFormat-stamp
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-update
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-download-err.log
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-download-Release-impl.cmake
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-patch
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-mkdir
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-done
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-install
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-download-out.log
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-configure
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-patch-info.txt
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-download
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-build
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-urlinfo.txt
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-update-info.txt
./ClangFormat-prefix/src/ClangFormat-stamp/ClangFormat-download-Release.cmake
./ClangFormat-prefix/src/ClangFormat
./ClangFormat-prefix/src/ClangFormat/clang-format
./ClangFormat-prefix/src/ClangFormat-build
./ClangFormat-prefix/tmp/ClangFormat-cfgcmd.txt
./ClangFormat-prefix/tmp/ClangFormat-mkdirs.cmake
./KWStyle/Utilities/boost/config/compiler/clang.hpp
./KWStyle/Utilities/boost/predef/compiler/clang.h
./CMakeFiles/ClangFormat.dir
./CMakeFiles/ClangFormat-complete
dzenan@corista:~/ITK-git-rel$

dzenanz · 2024-05-17T20:00:59Z

Yes, it gets build in the bin directory, under name clang-format-Linux (see above listing). It should be similar for Mac.

seanm · 2024-05-17T20:29:54Z

Yes, it gets build in the bin directory, under name clang-format-Linux (see above listing). It should be similar for Mac.

I've just built with VERBOSE=1 make and tried searching for keywords like clang and format but not found any evidence of it trying to build that. Do you have an exact string I might search for to see what's (not) happening?

dzenanz · 2024-05-17T20:53:47Z

Target is called ClangFormat, so maybe try make ClangFormat? Here are the relevant entries from my cache:

seanm · 2024-05-17T21:21:19Z

sean@hobgoblin ITK-big-bin % make ClangFormat
make: *** No rule to make target `ClangFormat'.  Stop.

But grepping ClangFormat in the ITK codebase I found this URL: https://data.kitware.com/api/v1/file/5d274e88877dfcc902effc47/download which appears to be the Mac clang-format8 executable! so I just downloaded that, copied it somewhere, did chmod +x and then git config clangFormat.binary /Users/sean/Downloads/clang-format8 and presto the ITK git hooks are now happy!

This is a fix about -Wmissing-variable-declarations warning of clang This is about the static case. A lot of variables were marked as warning due to non declaration. The namespace blocks of code were added when possible to fix. Otherwise it is static.

This is to fix the -Wmissing-variable-declarations warning of clang. This is simply related that the variable in question is not used in the file

seanm · 2024-05-24T17:09:29Z

Modules/Video/Core/test/itkTemporalProcessObjectTest.cxx

@@ -194,7 +194,7 @@ class CallRecord
 * Static list of CallRecord items representing the stack trace of
 * calls to GenerateData and TemporalStreamingGenerateData
 */
-std::vector<CallRecord> m_CallStack;
+static std::vector<CallRecord> m_CallStack;


It's orthogonal to this change, but this m_CallStack appears to be a plain old global variable, not a class instance variable. Should it have the m_ prefix?

I guess it should not.

What's the naming convention for plain old global variables then? I don't see the answer here: https://itk.org/Wiki/ITK/Coding_Style_Guide#Naming_Conventions

It should not have the "m_" prefix. It should be included in this PR.

andrei-sandor · 2024-05-24T17:26:14Z

Modules/Filtering/Path/test/itkContourExtractor2DImageFilterTest.cxx

For this one, I decided to keep it static since the variable that is static is used right after and there is no static to that one. I didn't want to let inside a big namespace to complicate more. Is it okay like this or I should create a large namespace?

hjmjohnson

@andrei-sandor Minor change needed. New static variable should have the m_ removed from the variable name. Will produce a followup PR to remove the m_

andrei-sandor · 2024-06-01T14:02:19Z

@hjmjohnson Yes sure, that's good for me.

dzenanz reviewed May 16, 2024

View reviewed changes

Examples/RegistrationITKv4/DeformableRegistration17.cxx Outdated Show resolved Hide resolved

dzenanz approved these changes May 17, 2024

View reviewed changes

github-actions bot removed the area:IO Issues affecting the IO module label May 21, 2024

andrei-sandor force-pushed the WmissingVariableDeclaration branch 3 times, most recently from ece4013 to acb3c5d Compare May 23, 2024 18:49

andrei-sandor added 2 commits May 24, 2024 12:58

COMP: Fix missing declarations of unused variables.

d69792e

This is to fix the -Wmissing-variable-declarations warning of clang. This is simply related that the variable in question is not used in the file

andrei-sandor force-pushed the WmissingVariableDeclaration branch from acb3c5d to d69792e Compare May 24, 2024 16:58

seanm reviewed May 24, 2024

View reviewed changes

andrei-sandor commented May 24, 2024

View reviewed changes

hjmjohnson approved these changes May 31, 2024

View reviewed changes

hjmjohnson merged commit 916e064 into InsightSoftwareConsortium:master May 31, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wmissing variable declaration #4676

Wmissing variable declaration #4676

andrei-sandor commented May 16, 2024 •

edited

dzenanz left a comment

seanm commented May 17, 2024

dzenanz commented May 17, 2024

seanm commented May 17, 2024

dzenanz commented May 17, 2024

dzenanz commented May 17, 2024

seanm commented May 17, 2024

dzenanz commented May 17, 2024

seanm commented May 17, 2024 •

edited

seanm May 24, 2024

dzenanz May 24, 2024

seanm May 24, 2024

hjmjohnson May 31, 2024 •

edited

andrei-sandor May 24, 2024

hjmjohnson left a comment

andrei-sandor commented Jun 1, 2024

Wmissing variable declaration #4676

Wmissing variable declaration #4676

Conversation

andrei-sandor commented May 16, 2024 • edited

dzenanz left a comment

Choose a reason for hiding this comment

seanm commented May 17, 2024

dzenanz commented May 17, 2024

seanm commented May 17, 2024

dzenanz commented May 17, 2024

dzenanz commented May 17, 2024

seanm commented May 17, 2024

dzenanz commented May 17, 2024

seanm commented May 17, 2024 • edited

seanm May 24, 2024

Choose a reason for hiding this comment

dzenanz May 24, 2024

Choose a reason for hiding this comment

seanm May 24, 2024

Choose a reason for hiding this comment

hjmjohnson May 31, 2024 • edited

Choose a reason for hiding this comment

andrei-sandor May 24, 2024

Choose a reason for hiding this comment

hjmjohnson left a comment

Choose a reason for hiding this comment

andrei-sandor commented Jun 1, 2024

andrei-sandor commented May 16, 2024 •

edited

seanm commented May 17, 2024 •

edited

hjmjohnson May 31, 2024 •

edited