Using the C++11 auto keyword in ITKv5 code

On recent patches to ITKv5 that I proposed, I was asked not to use the C++11 auto keyword. For example, in the following code:

const auto centerPoint = (*itCircles)->GetCenterPoint();

Which I then replaced by the following (hoping to get the patch accepted more easily):

const HoughTransformFilterType::CircleType::PointType centerPoint =
    (*itCircles)->GetCenterPoint();

Is there an agreement that the last line of code (without auto) is better than the fist one (with auto)? Because personally I would still prefer the first one.

In general, for a specific type T, the statement T x = f() might have an implicit type conversion. Whereas auto y = f() clearly just copies the return value of f() to y.

C++ guru Herb Sutter even recommends “AAA Style (Almost Always Auto)”:

1 Like

I like the non-auto way because it tells in the same line the type. With
auto, I need to look elsewhere for the type.

From my point of view, the first version with auto is better than the second, in terms of maintainability, reusability, but specially readability of the code. I think there is a common misconception with auto and type safety. auto is not like the any type from weakly typed languages. It guarantees the type safety even more strongly (without narrowing conversions) as you pointed out.

One of the cases where it should be forbidden or used with extreme caution is when type conversions are explicitly needed, for example, the Eigen math library which uses lazy evaluation for matrix operations.

In this sense, I don’t know if there are similar cases in ITK where type conversion must be explicit or we would get a surprising auto type.

1 Like

Please review this patch, which adds auto usage guidelines to the ITK Coding Style Guide.

This is consistent with the readability goals outlined in the style guide:

This chapter describes the ITK Coding Style. Developers must follow these conventions when submitting contributions to the toolkit.

The following coding-style guidelines have been adopted by the ITK community. To a large extent
these guidelines are a result of the fundamental architectural and implementation decisions made
early in the project. For example, the decision was made to implement ITK with a C++ core using
principles of generic programming, so the rules are oriented towards this style of implementation.
Some guidelines are relatively arbitrary, such as indentation levels and style. However, an attempt
was made to find coding styles consistent with accepted practices. The point is to adhere to a common
style to assist community members of the future to learn, use, maintain, and extend ITK. A common
style greatly improves readability.

and naming conventions:

Other general rules that must be followed in naming ITK constructs are:
• Underscores are not used (with the sole exception of enums and member variables).
• Variable names are chosen carefully with the intention to convey the meaning behind the code.
• Names are generally spelled out; use of abbreviations is discouraged. While this does result in
long names, it self-documents the code (e.g. use Dimension, point, size, or vector, instead
of D, pt, sz, or vec, respectively). Abbreviations are allowable when in common use, and
should be in uppercase as in RGB, or ID for “identifier”.)

and where VTK landed regarding auto.

It avoids duplicate type specification when the type is already specified on the right hand side of an assignment and enables use in range-based for loops, but it maintains readability by requiring explicit types otherwise that provide meaning and context.

1 Like

These guidelines seems good for ITK library code. However, it could be useful to distinguish these from recommendations for application code (that just uses ITK), where probably “almost-always auto” style applies. This simpler style could be used in ITK examples, where it may be more important to show how easy to use ITK.

4 Likes

I completely agree with @lassoan. ITK client code can be incredibly verbose. If you follow the examples, there are often #typedefs all over the place, even for types that are only used once. I have generally swapped to auto for these kinds of types, and now find it more readable and less error prone. In my opinion, typedefs are even worse than auto for the “having to look up what that type actually is” problem.

But yes, within ITK it is less clear-cut. Personally I am lazy, and auto saves me one heck of a lot of typing, so I tend to favour its use.

2 Likes

Yes, agreed that a more liberal use of auto in application code and examples makes it easier and quicker to write. I will make a note in the Style Guide patch to distinguish application code and examples.

Since ITK lines of code are read thousands of times more than it is written, explicit readable code saves much more time than the time required to write the code. As much as possible, the meaning of code should be evident from local context.

Very poor information on the intended meaning of the output:

auto localValue = crossCorrelation->GetOutput();

Poor information on the intended meaning of the output:

double localValue = crossCorrelation->GetOutput();

Good information on the intended meaning of the output:

ScalarMetricType localValue = crossCorrelation->GetOutput();

The variable name localValue is not conveying meaning! :smiley:
We do have the same problem without auto, I have seen this quite often:

CrossCorrelationFilterType::OutputType localValue = crossCorrelation->GetOutput();

What about?
auto crossCorrelationScalarResult = crossCorrelation->GetOutput()

We can provide meaning with the type, and/or the variable name.

But yes, I agree GetOutput() is too generic for auto to be clear there. It’s not the same as GetArray(), or similar. But I think that confusion happens with or without auto.

1 Like

Both the instance variable name and the type name can and should convey meaning, but they are not necessarily the same meaning. Types and instances are different and they should be named as appropriate, e.g.

ScalarMetricType localValue = [...]
ScalarMetricType globalValue = [...]

Scalar does not belong in the instance name – this is a description of the type, not the instance.

2 Likes

What do we think about auto usage in the following case:

ImageType::Pointer image = ImageType::New();

to

auto image = ImageType::New();

This is a case where the the type is “nearly” duplicated on the left and the right. I think declaring a typedef or using of the class is useful, but not having to define the pointer too would be very nice.

Perhaps the later could be done in the ITK examples?

1 Like

The current coding style guidelines patch suggests that auto should be used in this case, but we can make that more explicit.

2 Likes

Thank you guys, thanks Matt, for allowing more auto than before :smiley:

Hope you also allow auto to define a trailing return type, in those cases where specifying the return type before the function name (the old style) appears more cumbersome than after the function name (trailing return type). As I did here:

template< typename TInputPixelType, typename TOutputPixelType, typename TRadiusPixelType >
auto
HoughTransform2DCirclesImageFilter< TInputPixelType, TOutputPixelType, TRadiusPixelType >
::Circle::GetCenter() const
-> CenterType  // Trailing return type!
{
  return m_Center;
}

At my proposed patch ENH: Added HoughTransform2D Circle class, simplified GetCircles() return type line 54 of
http://review.source.kitware.com/#/c/23139/9/Modules/Filtering/ImageFeature/include/itkHoughTransform2DCirclesImageFilter.hxx

(Without using auto and trailing return type, the return type would have to be specified as
typename HoughTransform2DCirclesImageFilter< TInputPixelType, TOutputPixelType, TRadiusPixelType >::Circle::CenterType, that’s what looks cumbersome to me!)

Good point, Neils. Trailing return type has been added to the list.

1 Like

Thanks, Matt. Another valid auto use case, in my opinion, is to store a lambda expression in a variable, in order to use it multiple times:

double SumOfSquares(const double a, const double b)
{
    // Storing the lambda in a closure variable, 'square', in order to use it twice:
    auto square = [](const double x) { return x * x; };

    return square(a) + square(b);
}

In this case, an std::function variable could be used instead, but that would cause a performance penalty.

4 Likes

Yes, … added to the patch!

Note that @dzenanz is adding support to the new multi-threading framework for ITK 5.0 to elegantly accept lamda’s and std::function – exciting!

2 Likes