ENH: Add "Trailing Return Types" section

[N-Dekker]

Adds a guideline in accordance with commit InsightSoftwareConsortium/ITK@c54ad67
"COMP: Use trailing return type instead of typename + dependent type"

Discussed at pull request InsightSoftwareConsortium/ITK#2780
"Workaround VS2017 compile errors using-declarations"

[thewtex]

👍 💇

[jhlegarreta]

Thanks for doing this.

Can the clang style tool enforce these rules?

[jhlegarreta]

I would prefer avoiding the "simplest" and "shortest" terms/would prefer a clear/objective rule: e.g. when using language built-in types, use the return type before the function name.

A few reasons:

• I do not think contributors will be computing the length of the added characters/saying language built-in types is more clear IMO
• If the ITK PR was done automatically using a regex, the rules of that regex should apply here (i.e. there was no explicit modelling of "shortest"/"simplest").

[N-Dekker]

Thanks for your comments so far @thewtex and @jhlegarreta

For the record: an example of the VS2017 compile error that occurs when using the old (C++98) form for return types is here: InsightSoftwareConsortium/ITK#2759 (comment) Specifically:

template <typename T>
typename Derived<T>::NestedType
Derived<T>::MemberFunction()
{  // <==  error C2244: unable to match function definition to an existing declaration

Such an error can be avoided by using trailing return types.

Clang-tidy has two very different options:

• modernize-use-trailing-return-type: "Rewrites function signatures to use a trailing return type"
• fuchsia-trailing-return: "Functions that have trailing returns are disallowed, except for those using decltype specifiers and lambda with otherwise unutterable return types"

Interestingly fuchsia-trailing-return cannot be used, as long as we need to support VS2017.

I wouldn't mind some delay for this pull request. As long as we carefully avoid those specific VS2017 errors, we may also just continue doing what we always did, without a strict trailing-return-type rule.

[jhlegarreta]

Interestingly fuchsia-trailing-return cannot be used, as long as we need to support VS2017.

Not sure I see why.

As long as we carefully avoid those specific VS2017 errors,

I guess there is no guarantee for that and we have to rely on the CDash builds to spot those.

without a strict trailing-return-type rule.

IMO not having a tool to automatically do this will make the code inconsistent. e.g. think already to all the remotes, even more so when some of their parts are integrated into ITK. Although we as reviewers may spot the appearances, we are very, very likely to miss them, even more so not being used to using them.

If fuchsia-trailing-return cannot be used, then a pre-commit hook with the used regex may be enough for now.

[N-Dekker]

Interestingly fuchsia-trailing-return cannot be used, as long as we need to support VS2017.

Not sure I see why.

The fixes we committed to ITK for those VS2107 compile errors would be rejected by fuchsia-trailing-return. For example, looking at InsightSoftwareConsortium/ITK#2759 (comment) the following trailing-return-type is disallowed by fuchsia-trailing-return:

template <typename T>
auto
Derived<T>::MemberFunction() -> NestedType

But we do use trailing-return-types in such cases, to avoid those VS2017 errors! Catch-22!

So maybe we could postpone a decision on a trailing-return-type guideline for ITK until we have dropped VS2017, eventually 😺

Or otherwise we could do Clang-Tidy modernize-use-trailing-return-type! Some C++ users very much like to have trailing-return-types everywhere, as it is the most consistent approach.

[jhlegarreta]

But we do use trailing-return-types in such cases, to avoid those VS2017 errors! Catch-22!

OK.

So maybe we could postpone a decision on a trailing-return-type guideline for ITK until we have dropped VS2017, eventually smiley_cat

If the ITK PR gets merged, this one will need to be merged. Guidelines need to go hand in hand with the changes.

Using the Clang-Tidy trailing return type modification should be discussed in the ITK PR. If people are not for it, a pre-commit hook is the only solution for now as I see it.

Edit: I now realize that the ITK PR got merged a few days ago, sorry. I think the pre-commit hook would allow us to keep consistency.

[dzenanz]

Is this ready for "official review" and merging?

[N-Dekker]

Is this ready for "official review" and merging?

@dzenanz It is open for discussion, but Jon (@jhlegarreta) and I could not really get to an agreement. And honestly there appears no consensus amongst experienced C++ programmers in general, on whether to use trailing return types "whenever possible", or only "whenever necessary".

My proposal is still:

Whenever choosing between a trailing return type (as introduced with C++11) and the old form of having the return type before the function name (as was already supported by C++98), prefer the form that is the shortest, and the simplest.

Unfortunately, I'm not aware of any tool that automatically checks the code against my proposed rule.

Currently, ITK follows a similar (!) but slightly different rule: Use a trailing return type when it is necessary to avoid those VS2017 compilation errors. This rule is checked automatically at the dashboard, from time to time 😸

If we can't get to an agreement, it's OK to me to close the PR without merging.

[dzenanz]

Whenever choosing between a trailing return type (as introduced with C++11) and the old form of having the return type before the function name (as was already supported by C++98), prefer the form that is the shortest, and the simplest.

I agree with this. At least for the new code. If changing the old code is a manual work, we should not do it.

The trailing return types have access to the class' scope, and all its typedefs aliases, which frequently results in shorter type specification, not to mention avoidance of redundancy.

[dzenanz]

If both trailing and leading return type specifications have about the same length, we should prefer the classic (leading) one.

[jhlegarreta]

While the proposal and the merged style/compilation changes in InsightSoftwareConsortium/ITK@c54ad67 look valuable to me, as mentioned in this comment #162 (comment) adopting one style or the other is not enforced and, more importantly, relies on some requirement that is hard to fulfill, that is, computing the number of characters, or a subjective perception of what is "shortest/"simplest", or is ultimately upon the contributors' (or reviewers') commitment.

As said, if the InsightSoftwareConsortium/ITK@c54ad67 change could be formalized into a script (which may require some work, but I'd dare to say looks definitely doable), and that into a pre-commit hook, that would make things more consistent, and I'd definitely be for the change.

Also, note that ITK has 51 remote modules. It is not an easy task to keep the ITK changes in sync with them. Having automated ways to do so eases the task, scales easily (e.g. to future remote modules), and reduces the workload when the modules make their way into proper ITK.

Hope this makes sense.

[dzenanz]

We could formalize it like this, then:

Return type specification must be used if the return type references any of the type aliases of the class, and the classic (leading) type specification would require redundant repeat of their definition in the specification itself. Otherwise, the classic type specification must be used.

[N-Dekker]

@dzenanz Thanks for presenting a formalization, but personally I think the PR is clear enough. It also includes two examples, to clarify the rule:

In the following example, the old form is preferred over a trailing return type:

// Preferred:
template <typename T1, typename T2>
bool
AlmostEquals(T1 x1, T2 x2)

// Rather than:
template <typename T1, typename T2>
auto
AlmostEquals(T1 x1, T2 x2) -> bool

In the following example, a trailing return type is preferred over the old form:

// Preferred:
template <typename TValue, unsigned int VLength>
auto
FixedArray<TValue, VLength>::Size() const -> SizeType

// Rather than:
template <typename TValue, unsigned int VLength>
typename FixedArray<TValue, VLength>::SizeType
FixedArray<TValue, VLength>::Size() const

That's OK, right?

I think the main question is: do we want such a rule, or not? It won't be the end of the world, either way.

[dzenanz]

This section is short and clear enough. I vote in favor of this, for new code. If anyone wants to update the old code to the new style, that is welcome.

[hjmjohnson]

I think this is a sufficient step towards describing the intent of these proposed code changes.

The details and contention seem to be trying to find a more perfect solution for potential future problems. I am approving with the confidence that if some of those potential future problems materialize that this community will address those in an appropriate way.