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.

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

STYLE: Use auto for variables initialized by New() in Examples #2994

Conversation

jhlegarreta
Copy link
Member

@jhlegarreta jhlegarreta commented Dec 25, 2021

Replace initializations of the form T::Pointer var = T::New()
(sometimes starting with typename) with auto var = T::New() in Examples to
reduce code redundancy

In accordance with C++ Core Guidelines, August 19, 2021:
"ES.11: Use auto to avoid redundant repetition of type names"
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-auto

Following PR #2826
and commit 828453d.

PR Checklist

@github-actions github-actions bot added area:Examples Demonstration of the use of classes area:Python wrapping Python bindings for a class type:Style Style changes: no logic impact (indentation, comments, naming) labels Dec 25, 2021
@jhlegarreta
Copy link
Member Author

jhlegarreta commented Dec 25, 2021

A few notes:

  • I just did this because 5ab79bc was merged, and because it made no sense to keep the Examples inconsistent with the rest of the code.

  • @N-Dekker I noticed that your regexes do not cover cases where the New() statement is on a separate line, or does it? If not, there might be places across the code where the substitution was not applied.

    I did not find the regex doing that job so for this patch set I did it manually.

  • There are many SW Guide paragraphs that mention smart pointers as the return types just before the calls to New(), e.g.

//  A visitor implementation class can now be created using the normal
//  invocation to its \code{New()} method and assigning the result to a
//  \doxygen{SmartPointer}.

I think it is fair to leave them as they are without further explanation, since they are still accurate even if using auto.

If I'm not wrong the CIs do not build the examples, so I guess we will not know if I did not break anything until this gets merged and the regular builds report to CDash (did not build locally).

Edit: Examples are built for macOS CI 👌.

@jhlegarreta jhlegarreta force-pushed the UseAutoForSmartPointerInstatiationInExamples branch from 6bcf7cb to a945c77 Compare December 25, 2021 22:18
@N-Dekker
Copy link
Contributor

Very nice @jhlegarreta Indeed, the commit that I did (828453d), did not deal with the situation of having the New() statement on a separate line.

Just some comment on your commit message:

I would suggest to have a slightly different subject line, so that your commit can be distinguished more easily from mine. For example:

STYLE: Use auto for variables initialized by `New()` in Examples

Or just:

STYLE: Use auto for variables initialized by `New()` (follow-up)

The commit message says:

Following commit 5ab79bc.

That particular commit appears to produce a warning on GitHub, saying: "This commit does not belong to any branch ..." On the Git Bash command prompt, this particular commit cannot be found from a fresh ITK git clone, as I tried by doing git show 5ab79bc4149007a151c7e46735a49cffd5967a19. (It said "fatal: bad object".)

So I would suggest to instead mention the corresponding commit at the master branch: 828453d1bf61c487310d2d8c9570093e08798a40 (commit 828453d).

Also I think it would be nice to mention the corresponding pull request in the commit message: https://github.com/InsightSoftwareConsortium/ITK/pull/2826 (pull request #2826)

My two cents, Niels

Replace initializations of the form `T::Pointer var = T::New()`
(sometimes starting with `typename`) with `auto var = T::New()` in `Examples` to
reduce code redundancy

In accordance with C++ Core Guidelines, August 19, 2021:
"ES.11: Use `auto` to avoid redundant repetition of type names"
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-auto

Following PR InsightSoftwareConsortium#2826
and commit 828453d.
@jhlegarreta jhlegarreta force-pushed the UseAutoForSmartPointerInstatiationInExamples branch from a945c77 to 41cedd7 Compare December 26, 2021 01:54
@jhlegarreta jhlegarreta changed the title STYLE: Use auto for declaration of variables initialized by New() STYLE: Use auto for variables initialized by New() in Examples Dec 26, 2021
@jhlegarreta
Copy link
Member Author

jhlegarreta commented Dec 26, 2021

@N-Dekker done in 41cedd7.

Note that I essentially only looked at the very same PR you mentioned https://github.com/InsightSoftwareConsortium/ITK/pull/2826/commits, and as you can see the commit reference was accurate. The message you mention does appear on the web interface as well. For some reason the commit does not appear in the web history, but usually when a PR is merged from a fork, a new merge commit with the message Merge pull request (...) is added, and the original one is kept in the history. See the screenshots.

commit_auto_style_1

commit_auto_style_2

commit_auto_style_3

Not sure what's up with that commit, but it is done in any case.

Copy link
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.

👍

@N-Dekker
Copy link
Contributor

@jhlegarreta The commit you originally referred to (5ab79bc) belongs to PR #2826, indeed, so I guess that indeed, it will be kept in the history at github.com/InsightSoftwareConsortium/ITK for a long time. However, it won't be available "offline" at local ITK clones of end users. That's just why I prefer to mention the commit from the master branch (828453d), as that one is available at local ITK clones. And it may be available for an even longer time... 😃

Thanks for adjusting the commit message!

@dzenanz dzenanz merged commit 2f291f8 into InsightSoftwareConsortium:master Dec 27, 2021
@jhlegarreta jhlegarreta deleted the UseAutoForSmartPointerInstatiationInExamples branch December 27, 2021 15:59
@Leengit
Copy link
Contributor

Leengit commented Dec 29, 2021

We might be able to handle the multi-line statements with New() by first invoking clang-format with ColumnLimit: 10000 (or ColumnLimit: 0). Then they'll all be single line. After the auto regular-expression edit, then run clang-format again with default parameters. There could be side effects, but it might be worth a try to check it out.

N-Dekker added a commit to N-Dekker/ITKSphinxExamples that referenced this pull request May 19, 2022
Replaced initializations of the forms `T::Pointer var = T::New()` and
`typename T::Pointer var = T::New()` with `auto var = T::New()`, to reduce
code redundancy.

Using Notepad++ v8.3.3, Find in Files (Regular expression):

  Find what:  typename (\w+)::Pointer[ ]+(\w+) = \1::New\(\);
  Replace with:  auto $2 = $1::New\(\);

And then again without `typename`.

Following ITK pull request InsightSoftwareConsortium/ITK#2994
commit InsightSoftwareConsortium/ITK@2f291f8
"STYLE: Use `auto` for variables initialized by `New()` in Examples",
by Jon Haitz Legarreta Gorroño, merged on 27 December 2021.
tbirdso pushed a commit to tbirdso/ITKExamples that referenced this pull request May 20, 2022
Replaced initializations of the forms `T::Pointer var = T::New()` and
`typename T::Pointer var = T::New()` with `auto var = T::New()`, to reduce
code redundancy.

Using Notepad++ v8.3.3, Find in Files (Regular expression):

  Find what:  typename (\w+)::Pointer[ ]+(\w+) = \1::New\(\);
  Replace with:  auto $2 = $1::New\(\);

And then again without `typename`.

Following ITK pull request InsightSoftwareConsortium/ITK#2994
commit InsightSoftwareConsortium/ITK@2f291f8
"STYLE: Use `auto` for variables initialized by `New()` in Examples",
by Jon Haitz Legarreta Gorroño, merged on 27 December 2021.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Examples Demonstration of the use of classes area:Python wrapping Python bindings for a class type:Style Style changes: no logic impact (indentation, comments, naming)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants