Skip to content

STYLE: Prefer C++11 zero initializer to ZeroValue#3950

Merged
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:masterfrom
hjmjohnson:remove-ZeroValue-initializers
Mar 7, 2023
Merged

STYLE: Prefer C++11 zero initializer to ZeroValue#3950
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:masterfrom
hjmjohnson:remove-ZeroValue-initializers

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Prefer to use standard C++ language initializers to the itk::NumericValues<>::ZeroValue() initializers.

PR Checklist

Prefer to use standard C++ language initializers to
the itk::NumericValues<>::ZeroValue() initializers.
@hjmjohnson hjmjohnson requested a review from N-Dekker March 7, 2023 03:22
@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:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module type:Style Style changes: no logic impact (indentation, comments, naming) area:Numerics Issues affecting the Numerics module labels Mar 7, 2023
Copy link
Copy Markdown
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.

👍

Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker left a comment

Choose a reason for hiding this comment

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

Wonderful, thanks Hans! Did you use a regular expression to find and replace all those cases? If so, can you possibly still share the regular expression, either in the commit message or as a PR comment?

BTW, I would slightly reword the subject line, to mention {} explicitly, for example:

STYLE: Prefer C++11 `{}` initializer to NumericTraits::ZeroValue() call

But that's just a matter of style 😸 Approved anyway! 👍

N-Dekker added a commit to N-Dekker/ITKSoftwareGuide that referenced this pull request Mar 7, 2023
Dropped the requirement to use NumericTraits for all numeric data member initialization.

Following pull request InsightSoftwareConsortium/ITK#3950 by Hans Johnson.
@blowekamp
Copy link
Copy Markdown
Member

Will the {} initializer work for non-scalar pixel types too? Does this add new requirements for pixel types? As I recall some multi-component types itk::Size or itk::Index were defined as aggregate types. Will the {} initialization still zero the components?

@N-Dekker
Copy link
Copy Markdown
Contributor

N-Dekker commented Mar 7, 2023

Will the {} initializer work for non-scalar pixel types too? Does this add new requirements for pixel types? As I recall some multi-component types itk::Size or itk::Index were defined as aggregate types. Will the {} initialization still zero the components?

Hi @blowekamp Yes, itk::Size<N> and itk::Index<N> are properly zero-initialized by {}.

@N-Dekker
Copy link
Copy Markdown
Contributor

N-Dekker commented Mar 7, 2023

@blowekamp To elaborate on your question, with the following user-defined types (below here), the data members of instances of Aggregate and DefaultedConstructor are properly zero-initialized by the form T var{}. Only EmptyConstructor::m_Data3 is not initialized by EmptyConstructor var{}.

struct Aggregate
{
    int m_Data1;
};

class DefaultedConstructor
{
public:
  DefaultedConstructor() = default;
  int m_Data2;
};

class EmptyConstructor
{
public:
  EmptyConstructor() {}
  int m_Data3;
};

Aggregate aggregate{};  // m_Data1 is zero-initialized.
DefaultedConstructor defaultedConstructor{};  // m_Data2 is zero-initialized.
EmptyConstructor emptyConstructor{};  // m_Data3 is uninitialized!

See https://godbolt.org/z/xodK1T5Ys

@hjmjohnson hjmjohnson merged commit ef5cc8c into InsightSoftwareConsortium:master Mar 7, 2023
@hjmjohnson hjmjohnson deleted the remove-ZeroValue-initializers branch March 7, 2023 22:30
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Mar 9, 2023
Replaced initialization by `NumericTraits<T>::ZeroValue()` with empty `{}`
initializers. Did so by Replace in Files, using regular expressions:

    Find what: (\w+)([ ]+\w+) = NumericTraits<\1>::ZeroValue\(\);
    Replace with: $1$2{};

Follow-up to pull request InsightSoftwareConsortium#3950
commmit ef5cc8c
"STYLE: Prefer C++11 zero initializer to ZeroValue", by Hans Johnson.
hjmjohnson pushed a commit that referenced this pull request Mar 10, 2023
Replaced initialization by `NumericTraits<T>::ZeroValue()` with empty `{}`
initializers. Did so by Replace in Files, using regular expressions:

    Find what: (\w+)([ ]+\w+) = NumericTraits<\1>::ZeroValue\(\);
    Replace with: $1$2{};

Follow-up to pull request #3950
commmit ef5cc8c
"STYLE: Prefer C++11 zero initializer to ZeroValue", by Hans Johnson.
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:Examples Demonstration of the use of classes 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 area:Video Issues affecting the Video 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

Development

Successfully merging this pull request may close these issues.

5 participants