Skip to content

ENH: Add C++ script to add in-class {} member initializers to ITK#3935

Merged
dzenanz merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Maintenance-AddEmptyDefaultMemberInitializers.cxx
Mar 20, 2023
Merged

ENH: Add C++ script to add in-class {} member initializers to ITK#3935
dzenanz merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Maintenance-AddEmptyDefaultMemberInitializers.cxx

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker commented Feb 23, 2023

@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation labels Feb 23, 2023
Copy link
Copy Markdown
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.

Woo-hoo, CI was not run.

@jhlegarreta
Copy link
Copy Markdown
Member

Thanks for doing this @N-Dekker 💯, and also all the related PRs lately, and the PR to skip the CI for files that do no need to be tested.

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Feb 24, 2023

You're welcome @jhlegarreta I'm happy to see that it did not trigger a CI run 😃

You know I hesitated to place this script in the ITK git repository, because I did not want to add an extra maintenance burden. The script was meant to reduce maintenance in the first place! Once it is in here, people may do code clean-up's, fix typo's, upgrade the code to newer style guidelines or other compilers, etc. While I think ITK has more than enough code to maintain already!

As a compromise, could we agree that this script does not need to be at the master branch "forever"? Could it possibly be removed from the master after the next major release or so?

By the way, I may still make some small adjustments to the code, that's why it's still draft now.

@jhlegarreta
Copy link
Copy Markdown
Member

You know I hesitated to place this script in the ITK git repository, because I did not want to add an extra maintenance burden. The script was meant to reduce maintenance in the first place! Once it is in here, people may do code clean-up's, fix typo's, upgrade the code to newer style guidelines or other compilers, etc. While I think ITK has more than enough code to maintain already!

As a compromise, could we agree that this script does not need to be at the master branch "forever"? Could it possibly be removed from the master after the next major release or so?

Niels, we ourselves are trying to maintain code that we did not write, so anybody in the open source community that is interested in ITK is more than welcome to improve the script if they need it. But in the first place, it can be very useful for e.g. remote modules.

So even if you cannot devote time to maintain it further, I would keep it; it does not make sense to me to remove it for the next releases (for example, not all remotes may have time to apply the script by then).

By the way, I may still make some small adjustments to the code, that's why it's still draft now.

Fine, I just wanted to thank you for the effort of doing these things.

@blowekamp
Copy link
Copy Markdown
Member

Is there much overlap with this an the "use-default-member-init" functionality of clang tidy?
https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html

@N-Dekker Can you compare and contrast functionality?

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Feb 24, 2023

If we remove these scripts from master, they would be even harder to find than they are now. I vote to keep them where they are. We might even consider moving Maintenance folder to repository root, for improved visibility.

@N-Dekker
Copy link
Copy Markdown
Contributor Author

Is there much overlap with this an the "use-default-member-init" functionality of clang tidy?

Thanks for asking @blowekamp I think this script (AddEmptyDefaultMemberInitializers.cxx) is complementary to the clang tidy "use-default-member-init" option. The clang-tidy option moves existing (C++98/03 style) member initializers from constructors to the corresponding C++11/C++14 in-class data member declaration. While the script adds in-class {} initializers to data member declarations that did not yet have them.

Note that Hans (@hjmjohnson) has already run the clang tidy option extensively:

@N-Dekker N-Dekker force-pushed the Maintenance-AddEmptyDefaultMemberInitializers.cxx branch 4 times, most recently from e5cb2e2 to 0cf8243 Compare February 25, 2023 18:50
@N-Dekker
Copy link
Copy Markdown
Contributor Author

FYI Before merging, I would still like to extend the script to address lines of code that do not end with a semi-colon! Specifically:

int m_Data; // Some comment

Because of the trailing comment, this data member is still ignored now, even if it should have a {} initializer. To be continued... 😃

@N-Dekker N-Dekker force-pushed the Maintenance-AddEmptyDefaultMemberInitializers.cxx branch 2 times, most recently from a79f662 to f0189f3 Compare March 4, 2023 11:29
The script that was used for the following pull requests:

pull request InsightSoftwareConsortium#3851
"STYLE: Add in-class `{}` member initializers to objects created by New()"

pull request InsightSoftwareConsortium#3913
"STYLE: Add in-class `{}` initializers to classes with virtual functions"

pull request InsightSoftwareConsortium#3917
"STYLE: Add in-class `{}` initializers to classes with override = default"

pull request InsightSoftwareConsortium#3941
"STYLE: Add in-class `{}` member initializers to 3-letter data members"

pull request InsightSoftwareConsortium#3945
"STYLE: Add in-class `{}` member initializers having trailing comments"
@N-Dekker N-Dekker force-pushed the Maintenance-AddEmptyDefaultMemberInitializers.cxx branch from f0189f3 to 35426ac Compare March 6, 2023 13:17
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Mar 6, 2023
Ran "AddEmptyDefaultMemberInitializers" from pull request InsightSoftwareConsortium/ITK#3935 ("ENH: Add C++ script to add in-class `{}` member initializers to ITK") on the elastix source tree.
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Mar 6, 2023
Ran "AddEmptyDefaultMemberInitializers" from pull request InsightSoftwareConsortium/ITK#3935 ("ENH: Add C++ script to add in-class `{}` member initializers to ITK") on the elastix source tree.
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Mar 7, 2023
Ran "AddEmptyDefaultMemberInitializers" from pull request InsightSoftwareConsortium/ITK#3935 ("ENH: Add C++ script to add in-class `{}` member initializers to ITK") on the elastix source tree.
@N-Dekker N-Dekker marked this pull request as ready for review March 17, 2023 22:24
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.

Amazing to see C++ capable of what effectively only scripting languages could do in the past! 🎇

@dzenanz dzenanz merged commit ef40e98 into InsightSoftwareConsortium:master Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants