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

T.70 Prefer if constexpr to function template overloading #2099

Open
Eisenwave opened this issue Jun 23, 2023 · 15 comments
Open

T.70 Prefer if constexpr to function template overloading #2099

Eisenwave opened this issue Jun 23, 2023 · 15 comments

Comments

@Eisenwave
Copy link
Contributor

Eisenwave commented Jun 23, 2023

Here is a suggestion for a rule which encourages the use of if constexpr.
Not only do we repeat ourselves and keep logic less fragmented, we also save on compile times typically, because overload resolution is very expensive.

I'm not so sure if the recursive min example demonstrates it best, perhaps someone else can think of a better one.

T.70 Prefer if constexpr to function template overloading for short functions

Reason
  • Overloading is complicated, and overload resolution is expensive.
  • We repeat ourselves less.
  • Logic is often simplified and in one place.
Example, good
template<typename Head, typename... Tail>
auto min(const Head& head, const Tail&... tail)
{
    // can't be regular if-statement because else-block
    // would fail to compile for empty Tail
    if constexpr (sizeof...(Tail) == 0) { return head; }
    else {
        auto m = min(tail...);
        return head < m ? head : m;
    }
}
Example, bad
template<typename T>
auto min(const T& x) { return x; }

// bad: we've repeated the name "min", and must keep all function definitions in sync
template<typename Head, typename... Tail>
auto min(const Head& head, const Tail&... tail)
{
    auto m = min(tail...);
    return head < m ? head : m;
}
Note

Long function templates are difficult to read and understand. Consider F.3: Keep functions short and simple

Enforcement

Flag short, overloaded function templates.

@BenjamenMeyer
Copy link

The overload mentioned is useful. The types are not necessarily the same, and combining it down to a single function isn't necessarily useful either.

Overloading does come with a cost, but it's an acceptable cost in most cases, and often will be cleaned up during compile so runtime cost is usually negligible if any.

Let's not create practices that needlessly shill all things towards one methodology/type usage like in your examples.

@jwakely
Copy link
Contributor

jwakely commented Jun 26, 2023

The overload mentioned is useful.

How?

The types are not necessarily the same

Could you clarify what you mean here?

Overloading does come with a cost, but it's an acceptable cost in most cases, and often will be cleaned up during compile so runtime cost is usually negligible if any.

Let's not create practices that needlessly shill all things towards one methodology/type usage like in your examples.

(I don't think you mean "shill" here).

The rationale given makes sense, it's not "needlessly".

@cubbimew
Copy link
Member

At least in my team the accepted style is to prefer (tag-dispatched) overloading where possible, specifically to "keep functions short and simple" - we've seen an innocuous if constexpr or two grow into unreadable mess as code evolves and developers keep adding just one more else if constexpr.

I mostly see successful if constexpr in members of class templates.

@BenjamenMeyer
Copy link

Sorry for delay...

The overload mentioned is useful.
How?

Overloads handle different methods of calling so you can distinguish between how it is called and what parameter types are used in the call. Yes, templates generalize some of that, but not by much.

The types are not necessarily the same
Could you clarify what you mean here?

The bad example is already present as a counter and it's not really bad other than that it doesn't do what the requester wants.

In fact, the bad example can actually provide an implementation that has better performance than the good example since it has few parameters where with the multiple parameters the implementation has to necessarily loop over all the parameters instead of just doing a direct comparison.

Overloading does come with a cost, but it's an acceptable cost in most cases, and often will be cleaned up during compile so runtime cost is usually negligible if any.
Let's not create practices that needlessly shill all things towards one methodology/type usage like in your examples.
(I don't think you mean "shill" here).
The rationale given makes sense, it's not "needlessly".

The rational is pushing a specific design method that isn't necessarily very good. So yes it is needlessly shilling a specific methodology/practice.

That's not to say the pattern does make sense in some cases; however, it does not make sense in sufficient cases to be a standard here in the guide as it will end up leading to a lot of bad code because "following the guidelines".

@jwakely
Copy link
Contributor

jwakely commented Jun 28, 2023

Overloads handle different methods of calling so you can distinguish between how it is called and what parameter types are used in the call.

How is that not true for the variadic template? The number of types in the parameter pack tells you how it was called, and all the types are available.

In fact, the bad example can actually provide an implementation that has better performance than the good example since it has few parameters where with the multiple parameters the implementation has to necessarily loop over all the parameters instead of just doing a direct comparison.

Do you mean the "bad" one as shown can have better performance? How? It does the same thing. If you mean it could be implemented differently to have better performance, why couldn't that also be done in the "good" example's else branch?

I don't follow your reasoning at all, sorry.

@BenjamenMeyer
Copy link

Overloads handle different methods of calling so you can distinguish between how it is called and what parameter types are used in the call.

How is that not true for the variadic template? The number of types in the parameter pack tells you how it was called, and all the types are available.

In fact, the bad example can actually provide an implementation that has better performance than the good example since it has few parameters where with the multiple parameters the implementation has to necessarily loop over all the parameters instead of just doing a direct comparison.

Do you mean the "bad" one as shown can have better performance? How? It does the same thing. If you mean it could be implemented differently to have better performance, why couldn't that also be done in the "good" example's else branch?

I don't follow your reasoning at all, sorry.

The bad example has two implementations - one with a straight forward approach and nearly O(1); the other using variadic arguments to handle many parameters - O(n). The good example is always O(n) and has no possibility for O(1); adding another branch isn't going to resolve that.

Now if you consider the best operation time, the bad example still has a option that can be run in constant time with only a single comparison call (translates to a single assembler); your proposed fix to achieve this in the variadic argument method would be at minimum:

  • function call to get the length of the list - O(n)
  • comparison (constant)
  • comparison (constant)

So you've just made code perform worse even in the best possible code path all because of following a specific design pattern and always use variadic arguments. At minimum you've increased compile time if not also increasing run-time.

@jwakely
Copy link
Contributor

jwakely commented Jun 28, 2023

The bad example has two implementations - one with a straight forward approach and nearly O(1);

I have no idea what you're talking about. The first overload just returns its argument, that's not "a straight forward approach" to calculating the minimum of two values. It's dumb, and exists only because you have to terminate the recursion, which requires a separate function when using the "bad" style.

the other using variadic arguments to handle many parameters - O(n). The good example is always O(n) and has no possibility for O(1);

It uses if constexpr so when called with one argument, it returns that argument, exactly like the first overload of the "bad" example. When called with N arguments, it does exactly the same as the "bad" example: it recurses to find the minimum of the tail, then compares that with the head.

adding another branch isn't going to resolve that.

Adding another if constexpr branch has no runtime cost at all.

It's trivial to show that the codegen is identical: https://godbolt.org/z/W6jf87qqz

your proposed fix to achieve this in the variadic argument method would be at minimum

What proposed fix? I have no idea what you're talking about.

Again, the codegen for the "good" and "bad" examples is identical. The claimed difference is readability and maintenance. Your claims about performance are bogus.

Have you even read the examples?

@jwakely
Copy link
Contributor

jwakely commented Jun 28, 2023

And the same two pieces of code optimized (but with inlining disabled so it doesn't just get optimized to return 1;) are still identical: https://godbolt.org/z/GKGEzfcbf

@Eisenwave
Copy link
Contributor Author

function call to get the length of the list - O(n)

Are you confused by sizeof...(Tail) == 0? This is a constant expression, it doesn't have any runtime cost. I don't understand where you think the cost of getting the length of the list is.

At least in this case, the two functions should have identical codegen, except that the "good" example keeps all the logic in a single function, eliminating overload resolution.

@BenjamenMeyer
Copy link

At least in this case, the two functions should have identical codegen, except that the "good" example keeps all the logic in a single function, eliminating overload resolution.

There is always a penalty. Its just a matter of where.

@MikeGitb
Copy link

There is always a penalty. Its just a matter of where.

So where is the penalty here?

@Eisenwave
Copy link
Contributor Author

Eisenwave commented Jun 30, 2023

There is always a penalty. Its just a matter of where.

So where is the penalty here?

The penalty is that you will have to split this function up once it becomes too large. In that case, you would use overloading with tag dispatch possibly. Or alternatively, you could call other functions by name from inside of if constexpr blocks.

Anyhow, that's only a penalty when your function grows, in the form of some refactoring work. Other than that, I see it as strictly better than any other option:

  • you adhere to the DRY principle by not repeating the min name
  • you simplify the code by not having to manage multiple separate function signatures
  • you improve compile times (typically if constexpr is faster than going through overload resolution)
  • your code is much closer to a non-template recursive function, since you would typically implement base cases as an if-check)

@MikeGitb
Copy link

MikeGitb commented Jun 30, 2023

Just to clarify my personal stance: I totally agree with using constexpr if instead of overloading for most situations and especially for the example at hand to implement the base case of a recursive function.
I honestly don't understand, what @BenjamenMeyer is concerned about here.

I am a bit sceptical for cases, where two versions of a function handle fundamentally different Parameter types. Like "minimum of N separate parameters" and "minimum of the elements in a range". As preconditions, run-time complexity and maybe even other properties like constexprness and noexceptnes may vary significantly between those versions I think it's better to separate them behind separate function interfaces that can also be documented separately.

So I'm not sure if I want the compiler to warn on any overload set of function templates. The counterargument is of course that you probably should give those functions different names anyway if they are so different. I would have to look at more real-world cases to form an well founded opinion on there matter.

@Eisenwave
Copy link
Contributor Author

Eisenwave commented Jul 1, 2023

I am a bit sceptical for cases, where two versions of a function handle fundamentally different Parameter types. Like "minimum of N separate parameters" and "minimum of the elements in a range".

@MikeGitb that's not why we have two functions. The first overload is just a base case that we need for a single argument. Both functions are part of a variadic min(x, y, z, w, ...) function. The second overload calls min recursively, so we need this first base case overload to avoid infinite recursive instantiations.

if constexpr elegantly solves this because we can just put an if-statement into our function as a base case, which is simple and intuitive.

[...] and noexceptnes may vary significantly between those versions I think it's better to separate them behind separate function interfaces that can also be documented separately.

There is an argument to be had that creating separate functions assists in creating conditional noexcept specifications. However, noexcept-correct generic code is largely "correctness masturbation" outside of standard library code or something like boost. The average user can omit noexcept almost everywhere, maybe put it on move constructors, and they'll be perfectly fine.

What I'm trying to say is that noexcept-correctness is a pretty awful part of C++ no matter what idioms you use. It shouldn't factor into the advice of the guidelines.

@MikeGitb
Copy link

MikeGitb commented Jul 1, 2023

that's not why we have two functions. The first overload is just a base case that we need for a single argument

I know, that is exactly what I said in my first paragraph.

What I'm trying to say is that noexcept-correctness is a pretty awful part of C++ no matter what idioms you use. It shouldn't factor into the advice of the guidelines.

I disagree, but I don't think it's off big relevance for this rule one way or the other, so let's not get hung up on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants