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.

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

Replace Fill(T{}) with {} initialization, replace T var; var.Fill with auto var = T::Filled in tests #4887

Merged
merged 3 commits into from
Oct 21, 2024
Merged

Replace Fill(T{}) with {} initialization, replace T var; var.Fill with auto var = T::Filled in tests #4887

merged 3 commits into from
Oct 21, 2024

Conversation

N-Dekker
Copy link
Contributor

Replaced code of the form

Type var;
var.Fill(ElementType{});

with Type var{};

And specifically when having a local index or size variable, replaced code of the form

T var;
var.Fill(x);

with auto var = T::Filled(x);

Following C++ Core Guidelines, Oct 3, 2024, "Always initialize an object",
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-always

@N-Dekker
STYLE: Replace Fill(T{}) w/ {} initializer for local variables in tests
f36cd56 
Replaced code of the form

    Type var;
    var.Fill(ElementType{});

with `Type var{};`

Using Notepad++, Replace in Files, doing:

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

Follow-up to pull request InsightSoftwareConsortium#4881
commit 569a8b6
"STYLE: Replace Fill(0) with {} initializer for local variables in tests"
@github-actions github-actions bot added area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Bridge Issues affecting the Bridge module area:Core Issues affecting the Core module 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:Video Issues affecting the Video module area:Numerics Issues affecting the Numerics module labels Oct 19, 2024
@N-Dekker
Copy link
Contributor Author

@jhlegarreta I hope you like this PR! The adjustments might potentially prevent code analysis warnings about uninitialized variables. I added you as reviewer as you are very much involved with the tests of ITK.

@N-Dekker N-Dekker force-pushed the Replace-Fill-with-empty-initializer-or-Filled branch 2 times, most recently from fa1e339 to e991e27 Compare October 19, 2024 20:18
@N-Dekker
STYLE: Replace size.Fill with auto size = Size::Filled in tests
be250b8 
Replaced code of the form

    SizeType size;
    size.Fill(x);

with `auto size = SizeType::Filled(x);`

Following C++ Core Guidelines, Oct 3, 2024, "Always initialize an object",
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-always

Using Notepad++, Replace in Files, doing:

    Find what: ^([ ]+ )(.*Size.*[^ ])[ ]+(\w+);[\r\n]+ [ ]+\3\.Fill\(
    Replace with: $1auto $3 = $2::Filled\(
    Filters: itk*Test*.cxx
    [v] Match case
    (*) Regular expression

Removed "typename" keywords from `typename T::::SizeType::Filled` calls.

Follow-up to pull request InsightSoftwareConsortium#4881
commit 569a8b6
"STYLE: Replace Fill(0) with {} initializer for local variables in tests"
@N-Dekker
STYLE: Replace index.Fill with auto index = Index::Filled in tests
504d63b 
Replaced code of the form

    IndexType index;
    index.Fill(x);

with `auto index = IndexType::Filled(x);`

Following C++ Core Guidelines, Oct 3, 2024, "Always initialize an object",
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-always

Using Notepad++, Replace in Files, doing:

    Find what: ^([ ]+ )(.*::Index.*[^ ])[ ]+(\w+);[\r\n]+ [ ]+\3\.Fill\(
    Find what: ^([ ]+ )(IndexType)[ ]+(\w+);[\r\n]+ [ ]+\3\.Fill\(
    Replace with: $1auto $3 = $2::Filled\(
    Filters: itk*Test*.cxx
    [v] Match case
    (*) Regular expression

Removed "typename" keywords from `typename T::::IndexType::Filled` calls.

Follow-up to pull request InsightSoftwareConsortium#4881
commit 569a8b6
"STYLE: Replace Fill(0) with {} initializer for local variables in tests"
@N-Dekker N-Dekker force-pushed the Replace-Fill-with-empty-initializer-or-Filled branch from e991e27 to 504d63b Compare October 19, 2024 20:50
jhlegarreta
jhlegarreta approved these changes Oct 20, 2024
View reviewed changes
Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Looks good. Would be good to apply all of these to the ITK SW Guide (where appropriate, or the Fill method is not explicitly demonstrated), the ITK Sphinx Examples, and to the remotes.

@N-Dekker
Copy link
Contributor Author

Thanks @jhlegarreta I agree that it should then also be applied to the ITK SW Guide.

In general, I'm proposing for variable initializations:

  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

Would that also be OK to you?

@jhlegarreta
Copy link
Member

Would that also be OK to you?

Go for it if you have the bandwidth. Please, add the necessary explanation to the SWG and the necessary ITK Sphinx examples.

@N-Dekker
Copy link
Contributor Author

Please, add the necessary explanation to the SWG and the necessary ITK Sphinx examples.

Thanks for suggestion Jon. 👍 My intention is to first update the ITK source tree (ITK/Modules/itk*.*) accordingly, and then adjust the SWG and examples. But I'm not yet finished with the ITK source tree, I have to admit 🤷

@N-Dekker N-Dekker marked this pull request as ready for review October 20, 2024 12:12
thewtex
thewtex approved these changes Oct 21, 2024
View reviewed changes
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Thanks @N-Dekker !

@thewtex thewtex merged commit 19c7b5a into InsightSoftwareConsortium:master Oct 21, 2024
13 checks passed
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Oct 22, 2024
@N-Dekker
STYLE: Replace Fill calls with auto var = itk::MakeFilled<T> in tests
a9efd63 
Replaced code of the form

    T var;
    var.Fill(x);

with `auto var = itk::MakeFilled<T>(x);`

Following C++ Core Guidelines, Oct 3, 2024, "Always initialize an object",
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-always

Using Notepad++, Replace in Files, doing:

    Find what: ^( [ ]+)([^ ].*)[ ]+(\w+);[\r\n]+\1\3\.Fill\(
    Replace with: $1auto $3 = itk::MakeFilled<$2>\(
    Filters: itk*Test*.cxx
    [v] Match case
    (*) Regular expression

Follow-up to
- pull request InsightSoftwareConsortium#4881
- pull request InsightSoftwareConsortium#4884
- pull request InsightSoftwareConsortium#4887
@N-Dekker N-Dekker mentioned this pull request Oct 22, 2024
Merged
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Oct 24, 2024
@N-Dekker
STYLE: Replace Fill calls with auto var = itk::MakeFilled<T> in tests
7750331 
Replaced code of the form

    T var;
    var.Fill(x);

with `auto var = itk::MakeFilled<T>(x);`

Following C++ Core Guidelines, Oct 3, 2024, "Always initialize an object",
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-always

Using Notepad++, Replace in Files, doing:

    Find what: ^( [ ]+)([^ ].*)[ ]+(\w+);[\r\n]+\1\3\.Fill\(
    Replace with: $1auto $3 = itk::MakeFilled<$2>\(
    Filters: itk*Test*.cxx
    [v] Match case
    (*) Regular expression

Follow-up to
- pull request InsightSoftwareConsortium#4881
- pull request InsightSoftwareConsortium#4884
- pull request InsightSoftwareConsortium#4887
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Oct 25, 2024
@N-Dekker
STYLE: Replace Fill calls with auto var = itk::MakeFilled<T> in tests
d6c9eed 
Replaced code of the form

    T var;
    var.Fill(x);

with `auto var = itk::MakeFilled<T>(x);`

Following C++ Core Guidelines, Oct 3, 2024, "Always initialize an object",
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-always

Using Notepad++, Replace in Files, doing:

    Find what: ^( [ ]+)([^ ].*)[ ]+(\w+);[\r\n]+\1\3\.Fill\(
    Replace with: $1auto $3 = itk::MakeFilled<$2>\(
    Filters: itk*Test*.cxx
    [v] Match case
    (*) Regular expression

Follow-up to
- pull request InsightSoftwareConsortium#4881
- pull request InsightSoftwareConsortium#4884
- pull request InsightSoftwareConsortium#4887
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Oct 27, 2024
@N-Dekker
STYLE: Replace size.Fill with auto size = Size::Filled
9c6c613 
Using Notepad++, Replace in Files, doing:

    Find what: ^([ ]+ )(typename )?(.*Size.*[^ ])[ ]+(\w+);[\r\n]+ [ ]+\4\.Fill\(
    Replace with: $1auto $4 = $3::Filled\(
    Filters: itk*.h;itk*.hxx;itk*.cxx
    [v] Match case
    (*) Regular expression

Follow-up to pull request InsightSoftwareConsortium#4887
commit be250b8
"STYLE: Replace `size.Fill` with `auto size = Size::Filled` in tests"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Oct 27, 2024
@N-Dekker
STYLE: Replace Size var; var.Fill with auto var = Size::Filled
5b8e619 
Using Notepad++, Replace in Files, doing:

    Find what: ^([ ]+ )(typename )?(.*Size.*[^ ])[ ]+(\w+);[\r\n]+ [ ]+\4\.Fill\(
    Replace with: $1auto $4 = $3::Filled\(
    Filters: itk*.h;itk*.hxx;itk*.cxx
    [v] Match case
    (*) Regular expression

Follow-up to pull request InsightSoftwareConsortium#4887
commit be250b8
"STYLE: Replace `size.Fill` with `auto size = Size::Filled` in tests"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Oct 27, 2024
@N-Dekker
STYLE: Replace Index var; var.Fill with auto var = Index::Filled
1cda164 
Using Notepad++, Replace in Files, doing:

    Find what: ^([ ]+ )(typename )?(.*Index.*[^ ])[ ]+(\w+);[\r\n]+ [ ]+\4\.Fill\(
    Replace with: $1auto $4 = $3::Filled\(
    Filters: itk*.h;itk*.hxx;itk*.cxx
    [v] Match case
    (*) Regular expression

Follow-up to pull request InsightSoftwareConsortium#4887
commit 504d63b
"STYLE: Replace `index.Fill` with `auto index = Index::Filled` in tests"
@N-Dekker N-Dekker mentioned this pull request Oct 27, 2024
Merged
dzenanz pushed a commit that referenced this pull request Oct 29, 2024
@N-Dekker @dzenanz
STYLE: Replace Size var; var.Fill with auto var = Size::Filled
7fd1e73 
Using Notepad++, Replace in Files, doing:

    Find what: ^([ ]+ )(typename )?(.*Size.*[^ ])[ ]+(\w+);[\r\n]+ [ ]+\4\.Fill\(
    Replace with: $1auto $4 = $3::Filled\(
    Filters: itk*.h;itk*.hxx;itk*.cxx
    [v] Match case
    (*) Regular expression

Follow-up to pull request #4887
commit be250b8
"STYLE: Replace `size.Fill` with `auto size = Size::Filled` in tests"
dzenanz pushed a commit that referenced this pull request Oct 29, 2024
@N-Dekker @dzenanz
STYLE: Replace Index var; var.Fill with auto var = Index::Filled
3ff56ea 
Using Notepad++, Replace in Files, doing:

    Find what: ^([ ]+ )(typename )?(.*Index.*[^ ])[ ]+(\w+);[\r\n]+ [ ]+\4\.Fill\(
    Replace with: $1auto $4 = $3::Filled\(
    Filters: itk*.h;itk*.hxx;itk*.cxx
    [v] Match case
    (*) Regular expression

Follow-up to pull request #4887
commit 504d63b
"STYLE: Replace `index.Fill` with `auto index = Index::Filled` in tests"
@N-Dekker N-Dekker mentioned this pull request Nov 1, 2024
Merged
@N-Dekker
Copy link
Contributor Author

N-Dekker commented Nov 4, 2024 •
edited
Loading

Would be good to apply all of these to the ITK SW Guide (where appropriate, or the Fill method is not explicitly demonstrated), the ITK Sphinx Examples, and to the remotes.

@jhlegarreta Here is one for the ITK Sphinx Examples, to begin with:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhlegarreta jhlegarreta jhlegarreta approved these changes

@dzenanz dzenanz dzenanz approved these changes

@thewtex thewtex thewtex approved these changes

Assignees
No one assigned
Labels
area:Bridge Issues affecting the Bridge module 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:Python wrapping Python bindings for a class area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module 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
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@N-Dekker @jhlegarreta @thewtex @dzenanz