Skip to content

STYLE: Replace T var; var.Fill(x) with auto var = MakeFilled<T>(x) #5024

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 10, 2024
Merged

STYLE: Replace T var; var.Fill(x) with auto var = MakeFilled<T>(x) #5024

merged 4 commits into from
Dec 10, 2024

Conversation

@hjmjohnson
Copy link
Member

@hjmjohnson hjmjohnson commented Dec 8, 2024

Replace in Files, doing:
Find what: ^( [ ]+)([^ ].*)[ ]+(\w+);[\r\n]+\1\3.Fill(
Replace with: $1auto $3 = MakeFilled<$2>(

Simplify the creation of containers with initial
values.

PR Checklist

@hjmjohnson hjmjohnson added the type:Style Style changes: no logic impact (indentation, comments, naming) label Dec 8, 2024
@hjmjohnson hjmjohnson added this to the ITK 6.0 Beta 1 milestone Dec 8, 2024
@hjmjohnson hjmjohnson self-assigned this Dec 8, 2024
@hjmjohnson hjmjohnson force-pushed the replace-var.Fill-MakeFilled branch from 581dcca to c26c6f3 Compare December 9, 2024 00:17
@hjmjohnson hjmjohnson marked this pull request as draft December 9, 2024 00:21
@hjmjohnson
Copy link
Member Author

hjmjohnson commented Dec 9, 2024

Builds upon #5023

@hjmjohnson hjmjohnson mentioned this pull request Dec 9, 2024
Merged
2 tasks
N-Dekker
N-Dekker reviewed Dec 9, 2024
View reviewed changes
N-Dekker
N-Dekker reviewed Dec 9, 2024
View reviewed changes
@N-Dekker
Copy link
Contributor

N-Dekker commented Dec 9, 2024

Thanks Hans! Sorry, I overlooked the Examples when doing similar code clean-ups on ITK! I'm glad to see you do this follow-up. 👍

As I remarked in-line, I would prefer {} (rather than MakeFilled) for zero-initialization. As in the following pull request:

So I would suggest running the regex of pull request #4897 before adding the MakeFilled calls.

@N-Dekker
Copy link
Contributor

N-Dekker commented Dec 9, 2024

In general, I would suggest:

  1. To zero-fill a variable, use {}
  2. To fill a variable with arbitrary (non-zero) fill-value
    a. do auto var = T::Filled(fillValue) when the type is itk::Size<N> or itk::Index<N>
    b. do auto var = itk::MakeFilled<T>(fillValue) otherwise

As I wrote at #4887 (comment)

@hjmjohnson hjmjohnson force-pushed the replace-var.Fill-MakeFilled branch 2 times, most recently from 3e6e8ac to 32192c5 Compare December 9, 2024 15:34
@hjmjohnson
STYLE: Replace Fill(0) with {} initializer for local variables in tests
b0a8e85 
Another run to replaced code of the form

    T var;
    var.Fill(0);

with `T var{};`

Using Notepad++, Replace in Files, doing:

    Find what: ^( [ ]+[^ ].* )(\w+);[\r\n]+ [ ]+\2\.Fill\(0\);
    Replace with: $1$2{};
    Filters: itk*Test*.cxx
    [v] Match case
    (*) Regular expression

The empty initializer list `{}` effectively zero-fills those variables.
@hjmjohnson
STYLE: Prefer Filled over MakeFilled
1257ed2 
Prefer Filled over MakeFilled for initializing Size<T> and Index<T> types

Replace in Files, doing:
Find what: ^( [ ]+)(SizeType)[ ]+(\w+);[\r\n]+\1\3\.Fill\(
Replace with: $1auto $3 = $2::Filled\(
@hjmjohnson
ENH: Changes for preferring initialization to assignment.
be0568d 
This involved moving variable initialization to
the smallest possible scope.

The more locally scoped variables allow for future
automated code refactorings.
@hjmjohnson
STYLE: Manual review of .Fill([0-9]*) patterns
6746ae4 
Identified code style improvements based on manually
reviewing use of .Fill for assignment of values
when initialization could have been used.
@hjmjohnson hjmjohnson force-pushed the replace-var.Fill-MakeFilled branch from 32192c5 to 6746ae4 Compare December 9, 2024 17:08
@hjmjohnson hjmjohnson marked this pull request as ready for review December 9, 2024 17:08
@github-actions github-actions bot added area:Examples Demonstration of the use of classes type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Segmentation Issues affecting the Segmentation module labels Dec 9, 2024
@hjmjohnson hjmjohnson merged commit 9bc3ea0 into InsightSoftwareConsortium:master Dec 10, 2024
16 of 17 checks passed
@hjmjohnson hjmjohnson deleted the replace-var.Fill-MakeFilled branch December 10, 2024 02:12
@jhlegarreta
Copy link
Member

jhlegarreta commented Dec 10, 2024

Re: #5024 (comment) @N-Dekker Can this be put into the SWG and an ITK Example be added to the SphinxExamples repository?

@N-Dekker
Copy link
Contributor

N-Dekker commented Dec 10, 2024

@jhlegarreta Thanks, I can have a look at how to clearly put that guideline from #5024 (comment) into the SWG:

1. To zero-fill a variable, use `{}`

2. To fill a variable with arbitrary (non-zero) fill-value
   a. do  `auto var = T::Filled(fillValue)` when the type is `itk::Size<N>` or `itk::Index<N>`
   b. do `auto var = itk::MakeFilled<T>(fillValue)` otherwise

By the way, the most important guideline should of course just be: initialize, initialize, initialize, as soon as possible! The choice between {}, T::Filled(fillValue) and itk::MakeFilled<T>(fillValue) is more stylish. But I fully agree that it is worth mentioning in the SWG.

@jhlegarreta
Copy link
Member

jhlegarreta commented Dec 11, 2024

The dahsboard has started complaining about one or some of the changes introduced in this PR:
https://open.cdash.org/viewBuildError.php?buildid=10083433
It points to:
https://github.com/InsightSoftwareConsortium/ITK/blame/ddc8c11aee57e5ef262d6078eec09ba607594391/Modules/Nonunit/Review/test/itkImageFunctionTest.cxx#L137

@seanm
Copy link
Contributor

seanm commented Dec 12, 2024

I just tracked this down, but I see @jhlegarreta beat me to it. :)

@hjmjohnson you able to fix?

@dzenanz
Copy link
Member

dzenanz commented Dec 12, 2024

I made a PR for it: #5053.

N-Dekker added a commit to N-Dekker/ITKSoftwareGuide that referenced this pull request Dec 17, 2024
@N-Dekker
ENH: Add section "Initializing variables of fixed size array types"
5e5f2e5 
Suggested by Jon Haitz Legarreta Gorroño at
InsightSoftwareConsortium/ITK#5024 (comment)
@N-Dekker N-Dekker mentioned this pull request Dec 17, 2024
Merged
N-Dekker added a commit to N-Dekker/ITKSoftwareGuide that referenced this pull request Dec 17, 2024
@N-Dekker
ENH: Add section "Initializing variables of fixed size array types"
f10846c 
Suggested by Jon Haitz Legarreta Gorroño at
InsightSoftwareConsortium/ITK#5024 (comment)
@hjmjohnson hjmjohnson restored the replace-var.Fill-MakeFilled branch February 4, 2025 23:21
@hjmjohnson hjmjohnson deleted the replace-var.Fill-MakeFilled branch June 24, 2025 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@N-Dekker N-Dekker N-Dekker left review comments

@thewtex thewtex thewtex approved these changes

@dzenanz dzenanz dzenanz approved these changes

Assignees

@hjmjohnson hjmjohnson

Labels

area:Core Issues affecting the Core module area:Examples Demonstration of the use of classes area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Segmentation Issues affecting the Segmentation module type:Style Style changes: no logic impact (indentation, comments, naming) 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

Milestone

ITK 6.0 Beta 1

Development

Successfully merging this pull request may close these issues.

6 participants

@hjmjohnson @N-Dekker @jhlegarreta @seanm @dzenanz @thewtex