STYLE: Replace `static_cast<T>(X)` with `T{ X }` for any "all caps" `X` by N-Dekker · Pull Request #5337 · InsightSoftwareConsortium/ITK

N-Dekker · 2025-05-01T11:21:11Z

Replaced static_cast<(\w+)>$([A-Z_][A-Z_]+)$ with $1{ $2 }. According to common C++ naming conventions, these all "all caps" identifiers refer to constants, specified by preprocessor macro's, so these conversions are evaluated at compile-time.

Follow-up to pull request STYLE: Replace static_cast<T>(Dimension) with T{ Dimension } #5324 commit 31859d3

N-Dekker · 2025-05-02T07:17:42Z

/azp run ITK.Linux

dzenanz · 2025-05-02T13:07:17Z

Modules/Core/ImageAdaptors/test/CMakeFiles/ITKImageAdaptorsTestDriver.dir/itkComplexConjugateImageAdaptorTest.cxx.o -MF Modules/Core/ImageAdaptors/test/CMakeFiles/ITKImageAdaptorsTestDriver.dir/itkComplexConjugateImageAdaptorTest.cxx.o.d -o Modules/Core/ImageAdaptors/test/CMakeFiles/ITKImageAdaptorsTestDriver.dir/itkComplexConjugateImageAdaptorTest.cxx.o -c /home/runner/work/ITK/ITK/Modules/Core/ImageAdaptors/test/itkComplexConjugateImageAdaptorTest.cxx
/home/runner/work/ITK/ITK/Modules/Core/ImageAdaptors/test/itkComplexConjugateImageAdaptorTest.cxx: In function 'int itkComplexConjugateImageAdaptorTest(int, char**)':
/home/runner/work/ITK/ITK/Modules/Core/ImageAdaptors/test/itkComplexConjugateImageAdaptorTest.cxx:42:47: error: narrowing conversion of '2147483647' from 'int' to 'float' [-Wnarrowing]
   42 |     auto            randMax = float{ RAND_MAX };
      |                                               ^

N-Dekker · 2025-05-02T13:50:02Z

Modules/Core/ImageAdaptors/test/CMakeFiles/ITKImageAdaptorsTestDriver.dir/itkComplexConjugateImageAdaptorTest.cxx.o -MF Modules/Core/ImageAdaptors/test/CMakeFiles/ITKImageAdaptorsTestDriver.dir/itkComplexConjugateImageAdaptorTest.cxx.o.d -o Modules/Core/ImageAdaptors/test/CMakeFiles/ITKImageAdaptorsTestDriver.dir/itkComplexConjugateImageAdaptorTest.cxx.o -c /home/runner/work/ITK/ITK/Modules/Core/ImageAdaptors/test/itkComplexConjugateImageAdaptorTest.cxx
/home/runner/work/ITK/ITK/Modules/Core/ImageAdaptors/test/itkComplexConjugateImageAdaptorTest.cxx: In function 'int itkComplexConjugateImageAdaptorTest(int, char**)':
/home/runner/work/ITK/ITK/Modules/Core/ImageAdaptors/test/itkComplexConjugateImageAdaptorTest.cxx:42:47: error: narrowing conversion of '2147483647' from 'int' to 'float' [-Wnarrowing]
   42 |     auto            randMax = float{ RAND_MAX };
      |                                               ^

Interesting, thanks @dzenanz! This is exactly the purpose of T{ X }! Finding possibly unintended lossy conversions! 🎉

More interestingly, it appears that RAND_MAX fits in a 32-bit float on Windows and macos, but not on some Linux platforms! 🤔 On Windows/MSVC2022, RAND_MAX is only 32767, whereas it is 2147483647 on this Linux build, apparently!

I think it would be better to avoid RAND_MAX, as well as rand(), if we want to have the same result on all platforms 🤷

blowekamp · 2025-05-02T14:05:46Z

Modules/Core/ImageAdaptors/test/itkComplexConjugateImageAdaptorTest.cxx

  {
-    auto            randMax = static_cast<float>(RAND_MAX);
+    auto            randMax = float{ RAND_MAX };
    const PixelType pixel(static_cast<float>(rand()) / randMax, static_cast<float>(rand()) / randMax);


It does not look like anything is done with the value of the image before it is filled below. Maybe just use double, and then after division it can be reduced to float. I don't think it useful to working about system differences here.

Thanks @blowekamp but for now I just reverted back to static_cast<float>(RAND_MAX). Eventually I think we just shouldn't use the old rand() and RAND_MAX anymore. But that's for later 😺

I don't think this code block is a productive test. I question the usefulness of making it beautiful portable modern C++. Time may be better spent writing a replacement test.

I don't think this code block is a productive test. I question the usefulness of making it beautiful portable modern C++. Time may be better spent writing a replacement test.

Well, I would prefer using GoogleTest, of course. But can you please be more specific on why you think the code block is not productive?

It appears that the initial commit 75fd707 ("ENH: Added complex conjugate adaptor", Oct 26, 2011) already did so: first set a random number value for each pixel in the image, then fill the entire image buffer with PixelType( 2.0, -3.7 ):

ITK/Modules/Core/ImageAdaptors/test/itkComplexConjugateImageAdaptorTest.cxx

Lines 40 to 49 in 75fd707

itk::ImageRegionIterator< ImageType> iter( image, region );

for ( iter.GoToBegin(); iter != iter.End(); ++iter )

{

float randMax = static_cast< float >( RAND_MAX );

PixelType pixel( static_cast< float >( rand() ) / randMax,

static_cast< float >( rand() ) / randMax );

iter.Set( pixel );

}

image->FillBuffer( PixelType( 2.0, -3.7 ) );

Is that why you don't think it's a productive test?

Yes. The values are set and nothing is done with them. I suspect the initial test may have been with the random data, but it didn't work out.

While I think the test should work fine with arbitrary random input data, right?

@cquammen Hi Cory! Hope you're fine! We're just discussing your code from almost 14 years ago, I think 😃 I'm wondering, do you still remember why the ComplexConjugateImageAdaptor test first iteratively set the pixels of the image to random values, when it did image->FillBuffer directly afterwards anyway? What could have been the intention?

Most likely an incomplete change in my testing methodology :-) Feel free to remove.

Thanks @cquammen! Please check:

pull request STYLE: Make randomization input ComplexConjugateImageAdaptorTest useful #5342

Replaced `static_cast<(\w+)>$([A-Z_][A-Z_]+)$` with `$1{ $2 }`, using regular expressions. According to common C++ naming conventions, these all "all caps" identifiers refer to constants, specified by preprocessor macro's, so these conversions are evaluated at compile-time. Excluded replacing `static_cast<float>(RAND_MAX)` (as encountered in "itkComplexConjugateImageAdaptorTest.cxx") because this conversion is narrowing on some platforms. Specifically, on GCC `RAND_MAX` may be 2147483647, while it is only 32767 on MSVC2022. - Follow-up to pull request InsightSoftwareConsortium#5324 commit 31859d3 "STYLE: Replace `static_cast<T>(Dimension)` with `T{ Dimension }`"

thewtex · 2025-05-02T20:02:27Z

/azp run ITK.Linux

N-Dekker · 2025-05-05T09:52:29Z

/azp run ITK.Linux

Removed the `image->FillBuffer` call from this test, as it appeared to make the randomization of input pixel values useless. As noticed by Bradley Lowekamp at InsightSoftwareConsortium#5337 (comment) Cory Quammen (who originally wrote the test) also suggested removing this line of code.

N-Dekker · 2025-05-07T15:25:38Z

/azp run ITK.Linux

N-Dekker · 2025-05-07T15:50:33Z

@dzenanz @thewtex Do you have any clue why the CI is broken? It says:

2025-05-07T15:29:45.6443149Z CMake Error at /home/vsts/work/1/s/CMake/ExternalData.cmake:1171 (message):
2025-05-07T15:29:45.6443562Z   
2025-05-07T15:29:45.6444438Z 
2025-05-07T15:29:45.6445059Z   Object CID=bafybeifxqqmljz3yrqhrdv7vzrqrfkvs7aq2mx7mo3g3mjykqq3onlgh6i not
2025-05-07T15:29:45.6445869Z   found at:
2025-05-07T15:29:45.6446439Z 
2025-05-07T15:29:45.6447192Z     https://insightsoftwareconsortium.github.io/ITKTestingData/CID/bafybeifxqqmljz3yrqhrdv7vzrqrfkvs7aq2mx7mo3g3mjykqq3onlgh6i ("HTTP response code said error")
2025-05-07T15:29:45.6448294Z     https://data.kitware.com:443/api/v1/file/hashsum/CID/bafybeifxqqmljz3yrqhrdv7vzrqrfkvs7aq2mx7mo3g3mjykqq3onlgh6i/download ("HTTP response code said error")
2025-05-07T15:29:45.6449205Z     https://itk.org/files/ExternalData/CID/bafybeifxqqmljz3yrqhrdv7vzrqrfkvs7aq2mx7mo3g3mjykqq3onlgh6i ("HTTP response code said error")
2025-05-07T15:29:45.6449981Z     http://127.0.0.1:8080/ipfs/bafybeifxqqmljz3yrqhrdv7vzrqrfkvs7aq2mx7mo3g3mjykqq3onlgh6i ("Could not connect to server")
2025-05-07T15:29:45.6450817Z     https://ipfs.io/ipfs/bafybeifxqqmljz3yrqhrdv7vzrqrfkvs7aq2mx7mo3g3mjykqq3onlgh6i ("HTTP response code said error")
2025-05-07T15:29:45.6451825Z     https://gateway.pinata.cloud/ipfs/bafybeifxqqmljz3yrqhrdv7vzrqrfkvs7aq2mx7mo3g3mjykqq3onlgh6i ("HTTP response code said error")
2025-05-07T15:29:45.6452794Z     https://cloudflare-ipfs.com/ipfs/bafybeifxqqmljz3yrqhrdv7vzrqrfkvs7aq2mx7mo3g3mjykqq3onlgh6i ("Could not resolve hostname")
2025-05-07T15:29:45.6453593Z     https://dweb.link/ipfs/bafybeifxqqmljz3yrqhrdv7vzrqrfkvs7aq2mx7mo3g3mjykqq3onlgh6i ("HTTP response code said error")
2025-05-07T15:29:45.6453830Z

dzenanz · 2025-05-07T15:52:17Z

I don't know either.

blowekamp · 2025-05-07T16:57:02Z

$ git grep bafybeifxqqmljz3yrqhrdv7vzrqrfkvs7aq2mx7mo3g3mjykqq3onlgh6i
Modules/IO/GDCM/test/Input/Lily/HTJ2K-YBR_RCT.dcm.cid:bafybeifxqqmljz3yrqhrdv7vzrqrfkvs7aq2mx7mo3g3mjykqq3onlgh6i

That is a file I uploading related to DICOM and HTJ2K encoding. So it should be on the interplanetary filesystem?

Maybe the problematic file can be manually uploaded to Kitware's data repo?

dzenanz · 2025-05-07T17:25:01Z

@thewtex https://content-link-upload.itk.org/ seems to have dumbed up again. Alt link takes long to load, and has nearly no content.

thewtex · 2025-05-07T17:27:26Z

So it should be on the interplanetary filesystem?

I manually uploaded it.

/azp run ITK.Linux

Maybe the problematic file can be manually uploaded to Kitware's data repo?

Kitware has recommended not to use data.kitware.com, which may be taken offline.

@thewtex https://content-link-upload.itk.org/ seems to have dumbed up again. Alt link takes long to load, and has nearly no content.

I'll address this.

thewtex · 2025-05-07T17:29:18Z

/azp run ITK.Linux

blowekamp · 2025-05-08T11:31:28Z

@thewtex I am seeing this issue on the nightly dashboard too: https://open.cdash.org/viewBuildError.php?buildid=10385473

thewtex · 2025-05-08T13:24:40Z

@blowekamp thanks for the note. I added all your recent additions to the GitHub Pages resource:

CID/bafkreib5e6kni2tdbtaxpz2i77k7hp5gzkztppiuws3ov4yx4dswlna6p4
CID/bafybeifxqqmljz3yrqhrdv7vzrqrfkvs7aq2mx7mo3g3mjykqq3onlgh6i
CID/bafybeig7pgo3x55bfh47ymckelybsug7rv2nqwjic2gzurvng6o3vqoux4

thewtex · 2025-05-08T13:24:49Z

/azp run ITK.Linux

N-Dekker · 2025-05-10T20:02:21Z

/azp run ITK.Linux

Removed the `image->FillBuffer` call from this test, as it appeared to make the randomization of input pixel values useless. As noticed by Bradley Lowekamp at #5337 (comment) Cory Quammen (who originally wrote the test) also suggested removing this line of code.

thewtex · 2025-05-13T19:59:59Z

https://content-link-upload.itk.org/

A number of major updates were made, and it is now available at https://content-link-upload.netlify.app, and https://content-link-upload.itk.org/ will be available soon following a DNS fix.

N-Dekker marked this pull request as ready for review May 1, 2025 12:10

dzenanz approved these changes May 1, 2025

View reviewed changes

blowekamp reviewed May 2, 2025

View reviewed changes

N-Dekker force-pushed the Replace-static_cast-on-all-caps-macro-constants branch from a7a146e to 5730a69 Compare May 2, 2025 14:11

N-Dekker mentioned this pull request May 5, 2025

STYLE: Make randomization input ComplexConjugateImageAdaptorTest useful #5342

Merged

dzenanz merged commit 84cb6e8 into InsightSoftwareConsortium:master May 12, 2025
16 checks passed

N-Dekker mentioned this pull request May 14, 2025

STYLE: Replace rand() calls with std::mt19937 in ImageAdaptor test #5348

Merged

	itk::ImageRegionIterator< ImageType> iter( image, region );
	for ( iter.GoToBegin(); iter != iter.End(); ++iter )
	{
	float randMax = static_cast< float >( RAND_MAX );
	PixelType pixel( static_cast< float >( rand() ) / randMax,
	static_cast< float >( rand() ) / randMax );
	iter.Set( pixel );
	}

	image->FillBuffer( PixelType( 2.0, -3.7 ) );

Uh oh!

Conversation

N-Dekker commented May 1, 2025

Uh oh!

N-Dekker commented May 2, 2025

Uh oh!

dzenanz commented May 2, 2025

Uh oh!

N-Dekker commented May 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

N-Dekker May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewtex commented May 2, 2025

Uh oh!

N-Dekker commented May 5, 2025

Uh oh!

N-Dekker commented May 7, 2025

Uh oh!

N-Dekker commented May 7, 2025

Uh oh!

dzenanz commented May 7, 2025

Uh oh!

blowekamp commented May 7, 2025

Uh oh!

dzenanz commented May 7, 2025

Uh oh!

thewtex commented May 7, 2025

Uh oh!

thewtex commented May 7, 2025

Uh oh!

blowekamp commented May 8, 2025

Uh oh!

thewtex commented May 8, 2025

Uh oh!

thewtex commented May 8, 2025

Uh oh!

N-Dekker commented May 10, 2025

Uh oh!

Uh oh!

thewtex commented May 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

N-Dekker May 2, 2025 •

edited

Loading