STYLE: Use auto for declaration of variables initialized by New()

[N-Dekker]

Replaced initializations of the form T::Pointer var = T::New()
(sometimes starting with typename) with auto var = T::New(), 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

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

Find what:  typename (\w+)::Pointer[ ]+(\w+) = \1::New\(\);
Replace with:  auto $2 = $1::New\(\);
Filters: itk*.hxx;itk*.h;itk*.cxx

And then again without typename:

Find what:  (\w+)::Pointer[ ]+(\w+) = \1::New\(\);

Follow-up to commit de713e7
"STYLE: Use auto for variable creation", Hans Johnson (@hjmjohnson), 9 February 2018.

[SimonRit]

Dankjewel Niels! Can you quickly explain why, in your opinion, the new version has a "better" style than the older one? It is not obvious to me and it would be easier to adopt the new style if I'm convinced it's better.

[N-Dekker]

Dankjewel Niels! Can you quickly explain why, in your opinion, the new version has a "better" style than the older one? It is not obvious to me and it would be easier to adopt the new style if I'm convinced it's better.

@SimonRit Graag gedaan Simon! I think the use of auto (rather than explicitly spelling out the type name) is in general non-controversial whenever the type is entirely obvious from the right-hand-side of the initialization.

Personally I find initialization by auto much easier to read, in such cases:

auto interpolator = VariableVectorInterpolatorType::New();

versus:

typename VariableVectorInterpolatorType::Pointer interpolator = VariableVectorInterpolatorType::New();

Hans Johnson (@hjmjohnson) has already applied Clang-Tidy option modernize-use-auto before, with commit 8a2a15f "STYLE: Use auto for variable creation", 9 February 2018. This pull-request is basically a follow-up to his commit. It's just that Clang-Tidy does not recognize T::New() expressions, otherwise it would have included the changes I'm proposing here.

Does that make things more obvious to you?

[SimonRit]

Got it, thanks! The benefit is not obvious to me but that does not matter, it is just yet another indication that I'm aging.
Any reason to leave all the ::New() which do not contain typename (which can be found with grep -r New * | grep -v auto)?

[N-Dekker]

The benefit is not obvious to me but that does not matter, it is just yet another indication that I'm aging.

Actually as I'm aging myself, my typing slows down a little, and I appreciate avoiding redundant typing even more than before!

Having said so, I do believe that the new generation of C++ programmers (those who started learning C++ when C++11 was already standardized) has very much embraced C++11 auto!

Any reason to leave all the ::New() which do not contain typename (which can be found with grep -r New * | grep -v auto)?

Good catch @SimonRit ! An amend is underway, including those variable declarations that do not contain typename.

[Leengit]

This aging C++ programmer tries to avoid auto except where the explicit type is a huge mouthful such as with iterators or lambda expressions, or is operating system dependent; and I would be happy enough keeping typename VariableVectorInterpolatorType::Pointer.
(Or, I might use

using VariableVectorInterpolatorTypePointer = typename VariableVectorInterpolatorType::Pointer;
VariableVectorInterpolatorTypePointer interpolator{ VariableVectorInterpolatorType::New() };

)

My reasoning has to do with the fact that the compiler can catch a type mismatch in milliseconds (microseconds?) but it sometimes takes me hours to track down a bug caused by an unexpected type.

But this is not a show stopper -- unless you are persuaded too. 😄

[N-Dekker]

Thanks for your comment @Leengit

I would be happy enough keeping typename VariableVectorInterpolatorType::Pointer. (Or, I might use

using VariableVectorInterpolatorTypePointer = typename VariableVectorInterpolatorType::Pointer;
VariableVectorInterpolatorTypePointer interpolator{ VariableVectorInterpolatorType::New() };

)

Wow, so unfortunate to hear you prefer those two lines to a simple one-liner, using auto:

auto interpolator = VariableVectorInterpolatorType::New();

I can understand that using auto may be disputable for an arbitratry initialization, as in auto answer = Compute(42); However, the return type of VariableVectorInterpolatorType::New() should be obvious to any ITK developer. Redundantly spelling out the type name multiple times would more likely introduce typo's than prevent them, as far as I can see.

[Leengit]

Yes, I agree 110% that auto is much easier to read! And so long as no one ever accidentally changes the return type of any New() function or, alternatively, so long as we permanently have a ctest to check that that return type isn't changed, it is a slam dunk win.

FWIW, my favorite approach, for when I am unsure what the type should be, is:

auto returnValue = MyFunctionWithUnknownReturnType();
char ***z = returnValue;

When I compile that, my compiler tells me what type it is that it cannot convert to char ***. I read that type and, assuming it is consistent with my expectations, I can often copy and paste it over the auto. And if it is not a type that I was expecting then that is a future bug that is averted with the effort of a minute or two.

[seanm]

I appreciate avoiding redundant typing even more than before!

Not saying this is what you've done here, but, in general: this is the wrong thing to optimize for. Code is read much more than written, and readability should be favored over minimising typing. I'm generally not a fan of auto for this reason.

[N-Dekker]

Not saying this is what you've done here, but, in general: this is the wrong thing to optimize for. Code is read much more than written, and readability should be favored over minimising typing. I'm generally not a fan of auto for this reason.

Thanks for your comment @seanm Not saying that you're suggesting otherwise, but I do care very much about code readability. For example, by carefully choosing identifier names, by using functional abstraction, and by commenting code that might not be self-documenting. In this particular case, with this pull request I think readability is actually improved. Just like it is documented about the Clang-Tidy modernize-use-auto check (which was already applied to ITK, by Hans):

This check is responsible for using the auto type specifier for variable declarations to improve code readability and maintainability.

So really, I'm just doing my best to improve the code.

[N-Dekker]

For the record, it appears that this pull request is in accordance with the ITK Coding Style (from the ITK Software Guide), and specifically the section The auto Keyword, added by Matt McCormick (@thewtex) commit InsightSoftwareConsortium/ITKSoftwareGuide@51e9227 which says:

The auto keyword should be used to specify a type in the following cases:

• The type is duplicated on the left side of an assigment when it is mandated on the right side, e.g. when there is an explicit cast or initializing with new or ITK's ::New().
  ...

At https://github.com/InsightSoftwareConsortium/ITKSoftwareGuide/blob/v5.2.1/SoftwareGuide/Latex/Appendices/CodingStyleGuide.tex#L1507-L1511

So if you agree, please say so by approving this pull request 😃

[phcerdan]

I think this is one of those perfect spots for auto, the type can be inferred by the reader in the same line.

[N-Dekker]

I think this is one of those perfect spot for auto, the type can be inferred by the reader in the same line.

@phcerdan Thanks Pablo! I saw that you originally suggested to Matt to include ITK's New() with the auto guideline, and I'm glad you did 😃

[jhlegarreta]

I will myself need to get used to it, but IMHO this improves readability.

As for the ITK SW Guide commit pointed in #2826 (comment):

• Adding examples to each of the points in the The auto Keyword section would be extremely helpful and maybe enlightening (if compared to not using auto).
• Besides the examples whose code is automatically taken from the ITK code base, the ITK SW Guide contains code blocks interspersed across the LaTeX source, and those should be changed too to avoid confusing contributors.

Thanks.

[N-Dekker]

@jhlegarreta Thanks for your approval Jon!

Besides the examples whose code is automatically taken from the ITK code base, the ITK SW Guide contains code blocks interspersed across the LaTeX source, and those should be changed too to avoid confusing contributors.

Addressed by pull request InsightSoftwareConsortium/ITKSoftwareGuide#163 please check!