Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Use a macro to print boolean objects #3908

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhlegarreta
Copy link
Member

@jhlegarreta jhlegarreta commented Feb 4, 2023

Use a macro to print boolean objects. Avoids boilerplate code.

Appearances of the form:

os << indent << "{name}: " << (m_{name} ? "On" : "Off") << std::endl;

and

os << indent << "{name}: " << (this->m_{name} ? "On" : "Off") << std::endl;

are substituted by `itkPrintBooleanMacro({name});. The cases where the
statement is broken into multiple lines were also considered.

Add the script used to perform automatically the substitution.

PR Checklist

@github-actions github-actions bot added type:Enhancement Improvement of existing methods or implementation area:Core Issues affecting the Core module labels Feb 4, 2023
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, the instances should be found and replaced 😄

@github-actions github-actions bot added the type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots label Feb 9, 2023
@jhlegarreta
Copy link
Member Author

jhlegarreta commented Feb 9, 2023

The expression
os << indent << "(.*): " << \(m_([a-zA-z]+) \? "On" : "Off"\)((\r\n|\r|\n).*| )<< std::endl;\n
works correctly in https://regex101.com/ to capture the relevant variable names in

  os << indent << "ForwardAzimuthElevationToPhysical: " << (m_ForwardAzimuthElevationToPhysical ? "On" : "Off")
     << std::endl;
  os << indent << "Minimize: " << (m_Minimize ? "On" : "Off") << std::endl;

But I cannot figure out how to get the multi-line case using sed or perl:
https://github.com/InsightSoftwareConsortium/ITK/blob/7b29032604fb04ce1b8107b260b71825007d1d60/Utilities/Maintenance/ReplaceBooleanPrintMacro.sh

This one works for both cases without capturing works:

perl -0777 -pe 's/os << indent << "ForwardAzimuthElevationToPhysical: " << \(m_ForwardAzimuthElevationToPhysical \? "On" : "Off"\)((\r\n|\r|\n).*| )<< std::endl;/replacetext/g' input.txt

But then again I cannot figure out how to capture the variable names into groups.

I could fix the multi-line ones manually, but I prefer to have a solution. Suggestions on the regex are welcome.

Edit: OK, this seems to work for both cases:

perl -0777 -pe 's/os << indent << "(.*): " << \(m_([a-zA-z]+) \? "On" : "Off"\)((\r\n|\r|\n).*| )<< std::endl;/itkPrintBooleanMacro\(\2\);/g' input.txt

Will use it in the script and see what happens when applying to the code.

@hjmjohnson
Copy link
Member

@jhlegarreta Is this ready to be pushed forward ?

@jhlegarreta
Copy link
Member Author

@hjmjohnson Will give it a try over the weekend.

@jhlegarreta
Copy link
Member Author

@hjmjohnson @seanm After all the no-op, semicolon, etc. discussions and changes lately, before I make the changes, is the macro correctly defined as is now?
https://github.com/InsightSoftwareConsortium/ITK/pull/3908/files#diff-635df68290f802f4b0b60c34e44e1c4b57dfc0c623a6c29efd9919624d31e399R1276

Is the ending semicolon after the macro correct in?
https://github.com/InsightSoftwareConsortium/ITK/pull/3908/files#diff-35939c01a094d5166fd7dd2dfc6ed059ec0e6f7184b3c0100ccbb2b7cbf80182R38

@jhlegarreta
Copy link
Member Author

@hjmjohnson I have the script working locally, tested on an example file. Please have a look at the questions in #3908 (comment) so that I can make the appropriate changes to the macro and the replacement statement if needed. Once those made, and #4763 merged, I will run the script and force push.

@jhlegarreta jhlegarreta force-pushed the UseAMacroToPrintBooleanObjects branch from 7b29032 to 7ca2b7d Compare July 3, 2024 23:53
@github-actions github-actions bot added type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Numerics Issues affecting the Numerics module labels Jul 3, 2024
@jhlegarreta jhlegarreta marked this pull request as ready for review July 3, 2024 23:54
@jhlegarreta
Copy link
Member Author

Went on and made the changes. If CIs come green this is ready to go.

@jhlegarreta jhlegarreta force-pushed the UseAMacroToPrintBooleanObjects branch from 7ca2b7d to 674a60b Compare July 4, 2024 00:13
@jhlegarreta
Copy link
Member Author

jhlegarreta commented Jul 4, 2024

Well, I included the file where the macro definition dwells just in case:
https://github.com/InsightSoftwareConsortium/ITK/pull/3908/files#diff-60c38f29d2b7fdf51351001d8746684f22860e7eda2a7efea0405c6d0445bbc5R35

But it does not solve the issue:
https://open.cdash.org/viewBuildError.php?buildid=9734250

Both ivars are declared in itkProcessObject.h. Also I do not understand why this does not happen for all the rest of *.hxx/*.cxx files or in the appearances of itkPrintSelfObjectMacro. The definition of the macro should be replaced when the code is compiled, so the names of the identifiers should be changed to m_{name} in the statements and the compiler should not complain as far as I understand.

Use a macro to print boolean objects. Avoids boilerplate code.

Appearances of the form:
```
os << indent << "{name}: " << (m_{name} ? "On" : "Off") << std::endl;
```

and
```
os << indent << "{name}: " << (this->m_{name} ? "On" : "Off") << std::endl;
```

are substituted by `itkPrintBooleanMacro({name});. The cases where the
statement is broken into multiple lines were also considered.

Add the script used to perform automatically the substitution.
@jhlegarreta jhlegarreta force-pushed the UseAMacroToPrintBooleanObjects branch from b397b7e to 41a3798 Compare July 4, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants