-
-
Notifications
You must be signed in to change notification settings - Fork 671
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
WIP: STYLE: Use ImageRegion auto [index, size] = region
structured bindings
#4379
WIP: STYLE: Use ImageRegion auto [index, size] = region
structured bindings
#4379
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Remove now unused type definitions to get rid of those warnings for a clean CI.
8ed2f28
to
9d78904
Compare
auto [index, size] = region
structured bindingsauto [index, size] = region
structured bindings
9d78904
to
7d1e437
Compare
FYI, I'm considering to postpone this pull request, for the moment. I think it's a nice "proof of principle" on how to use structured bindings with For example, the following line may trigger such a warning (lnt-accidental-copy, AUTO_CAUSES_COPY):
The warning would suggest doing the structured bindings "by reference" instead:
But I do not blindly want to follow their suggestion to replace copying with referencing. For a rather small object like I'm considering a workaround for those warnings, but I'm not sure yet. 🤷 Update, 3 January, 2024: A workaround against those potential warnings might be coming from the following pull request Which would allow: |
Replaced initializations of the form: SizeType size = region.GetSize(); IndexType index = region.GetIndex(); By using structured binding of the form `auto [index, size] = region`, as was introduced by pull request InsightSoftwareConsortium#4367 commit 72aa9a6 "ENH: `ImageRegion` support C++17 structured binding" Did so by Notepad++ Replace in Files, using regular expressions: Find what: ^([ ]+)const (.*)\w.+[ ]+(\w+) = (.+)\.GetIndex\(\);\r\n\1const \2\w+[ ]+(\w+) = \4\.GetSize\(\);$ Replace with: $1const auto [$3, $5] = $4; Find what: ^([ ]+)const (.*)\w.+[ ]+(\w+) = (.+)\.GetSize\(\);\r\n\1const \2\w+[ ]+(\w+) = \4\.GetIndex\(\);$ Replace with: $1const auto [$5, $3] = $4; Find what: ^([ ]+)(.*)\w.+[ ]+(\w+) = (.+)\.GetIndex\(\);\r\n\1\2\w+[ ]+(\w+) = \4\.GetSize\(\);$ Replace with: $1auto [$3, $5] = $4; Find what: ^([ ]+)(.*)\w.+[ ]+(\w+) = (.+)\.GetSize\(\);\r\n\1\2\w+[ ]+(\w+) = \4\.GetIndex\(\);$ Replace with: $1auto [$5, $3] = $4; Manually added `const` to bindings in `SetBound(const SizeType &)` member functions, excluded `ImageIORegion`, and removed `Index` and `Size` type aliases that are no longer used after this commit.
Added `const`, whenever the values of the bindings were not modified, after the declaration.
7d1e437
to
b4efee3
Compare
Removed those local "region" variables, as they appear that appear no longer useful.
Basically I think this pull request is OK, but if it gets merged, we might still get into a discussion on
|
Replaced initializations of the form:
By using structured binding of the form
auto [index, size] = region
, as was introduced by pull request #4367 commit 72aa9a6 "ENH:ImageRegion
support C++17 structured binding"Did so by Notepad++ Replace in Files, using regular expressions:
Manually added
const
whenever the values of the bindings were not modified, excludedImageIORegion
, and removedIndex
andSize
type aliases that are no longer used after this commit.When reviewing, it may be helpful to ignore whitespace changes: https://github.com/InsightSoftwareConsortium/ITK/pull/4379/files?w=1