search

InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.37k stars 660 forks source link

COMP: -Wextra-semi new fix proposed #4724

Closed andrei-sandor closed 1 day ago

andrei-sandor commented 2 weeks ago

Decided to remove semicolon from struct and not from where the function-like macro is used. Is this a fix that is acceptable? Can we proceed with similar fixes?

seanm commented 2 weeks ago

@hjmjohnson is this pattern fine, or do you even want a superfluous static_assert in this kind of case? We'd like to be sure what you'll accept before repeating the pattern.

seanm commented 1 week ago

OK, so as this is fine, we will update this PR to apply the same pattern...

andrei-sandor commented 1 week ago

@hjmjohnson We have committed new changes. However, we would like to know if we can proceed with adding ITK_MACROEND_NOOP_STATEMENT to macros like itkBooleanMacro. I know that in the old PR, you mention that it is not needed, but we saw some cases where the NOOP statement is not used.

hjmjohnson commented 1 week ago

@hjmjohnson We have committed new changes. However, we would like to know if we can proceed with adding ITK_MACROEND_NOOP_STATEMENT to macros like itkBooleanMacro. I know that in the old PR, you mention that it is not needed, but we saw some cases where the NOOP statement is not used.

Please do use the ITK_MACROEND_NOOP_STATEMENT at the end of 

itkBooleanMacro 
itkGPUKernelMacro
itkForLoopAssignmentMacro
itkForLoopRoundingAndAssignmentMacro

and others that would otherwise complain about unnecessary ';'

Thank you!

hjmjohnson commented 4 days ago

@andrei-sandor @seanm Is this ready for review?

seanm commented 3 days ago

@andrei-sandor @seanm Is this ready for review?

It is ready for merge in our opinion. There are still more -Wextra-semi to fix, but may as well get this next tranche in.

seanm commented 1 day ago

Would be nice to merge this, so that it doesn't conflict again...