Improve and reduce usage of universal reference and forward (WIP)

[TonyXiang8787]

Background

This PR fixes a code quality problem. In the code base, std::forward is excessively used. Most of the usage is not justified, but merely to satisfy the clang-tidy and sonar rules. This PR removes those usage and adjusts the rules.

Functor object as function argument should be passed by value

One misuse of std::forward is wrongly declaring functor object in function argument as universal reference. See below:

template <class Functor, class... Args>
decltype(auto) call_functor(Functor&& f, Args&&... args) {
    return std::forward<Functor>(f)(std::forward<Args>(args)...);
}

The CPP guideline is clear: Function objects should be cheap to copy (and therefore passed by value).

Defining functor argument as reference is not correct. The PR changes this back to pass by value. See below. Note the declaring universal reference for args and use std::forward on args is correct, which is actually the only correct usage of std::forward in PGM code base.

template <class Functor, class... Args>
decltype(auto) call_functor(Functor f, Args&&... args) {
    return f(std::forward<Args>(args)...);
}

Universal reference is used as capture temporary proxy object, it should not be forwarded

The universal reference (or the confusing name forwading reference) is not only meant to be used when you want to do perfect forwarding. It has another major use-case: capturing temporary proxy object. The proxy object can be:

• A eigen expression template
• A std::ranges::views which is constructed on the fly
• When proxy-like iterator object which constructs the underlying value on the fly when dereference, thus the dereference function return is by value.
• In deducing this functions, the Self&& is declared as universal reference to be able to capture a temporary created object.

In all this cases, nothing will need to be forwarded. Thus usage of std::forward does not make sense. The clang-tidy and sonar unfortunatley cannot detect this use-case, and raise false warning.

template <std::ranges::viewable_range ElementGroups>
// universal reference is needed to capture proxy object, a range view
constexpr auto sparse_encode(ElementGroups&& element_groups, Idx num_groups) {
    // ...
    // use of forward is not justified, it is there to only please clang-tidy
    auto element_groups_view = std::views::all(std::forward<ElementGroups>(element_groups));
    // ...
}

We should just remove the std::forward here.

// universal reference is needed to capture proxy object, a range view
constexpr auto sparse_encode(std::ranges::viewable_range auto&& element_groups, Idx num_groups) {
    // ...
    // Do not use forward. Do not make ugly code to please clang-tidy.
    auto element_groups_view = std::views::all(element_groups);
    // ...
}

When to use std::forward? Why clang-tidy complains so much?

std::forward is only justified if we expect that the follow-up function has potentially a rvalue reference overload which could consume the object. This is relatively rare in PGM. On the other hand, the use-case of universal reference to capture temporary proxy object is huge in PGM.

Unfortunately, both of them do not have good mechanism to detect the other legitimate use-case of universal reference. This results in many false warning from clang-tidy and sonar. This PR proposes to silence this rule for both clang-tidy and sonar globally. Silencing it locally would create too much //NOLINT comment which hams readability and again just trying to please clang-tidy.

[mgovers]

i would argue against globally disabling the check of both clang-tidy and sonar cloud. i get that there are multiple separate reasons why one would use it, and i get that it clutters the codebase, but i generally am of the opinion that the people working for sonar and clang are probably more aware of the details etc. than we are. also, even within the maintainer team, not everyone is as aware as you are as to when which use case is used. IMO, we should better be safe than sorry.

[TonyXiang8787]

i would argue against globally disabling the check of both clang-tidy and sonar cloud. i get that there are multiple separate reasons why one would use it, and i get that it clutters the codebase, but i generally am of the opinion that the people working for sonar and clang are probably more aware of the details etc. than we are. also, even within the maintainer team, not everyone is as aware as you are as to when which use case is used. IMO, we should better be safe than sorry.

Replying to your general comments @mgovers. The actual legitimate use of std::forward in the entire PGM code base is only this:

template <class... Args>
decltype(auto) some_func(Args&&... args) {
    return some_other_func(std::forward<Args>(args)...);
}

Even for this case, missing std::forward will only cause real performance problem if some_other_func actually has a rvalue overload which consumes (takes the ownership) the (possibly big) input parameter.

So the proposal to disable rules globally is based on three major facts:

1. The places where use of std::forward is actually making sense are overwhelmingly obvious for the context of PGM code base. We are not creating arbitrary places where you need a std::forward. The argument that normal developers will regularly miss that obvious use of std::forward is weak.
2. Even for those cases, the consequence of missing a std::forward is near to zero. It only matters if the subsequent function has a rvalue overload taking over the resource ownership.
3. Based on the two points above, the benefit of avoiding the hypothetical problems as stated in two points above by enabling clang-tidy and sonar warning, simply does not outweigh the drawback of decrease of code readability and decrease (external) contributor motivation when they see full of //NOLINT everywhere.

And no, for this particular case, sonar and clang-tidy is wrong in big way. They simply cannot distinguish difference use-cases of universal reference in a deterministic way.

[figueroa1395]

@mgovers @TonyXiang8787 Can we do a knowledge sharing for everyone before we merge this? I can prepare it if so. It's good to be aligned first and make sure we all understand the subtleties well.

[TonyXiang8787]

@mgovers @TonyXiang8787 Can we do a knowledge sharing for everyone before we merge this? I can prepare it if so. It's good to be aligned first and make sure we all understand the subtleties well.

The intention is indeed to discuss and agree with the maintainers, then merge (possibly with adjustments).

[figueroa1395]

A few additional points to add to this discussion:

• std::ranges::viewable_range requires perfect forwarding (together with std::views::all) to work correctly with std::vector and std::span (with all their &, const, etc. versions).
• According to the CPP guidelines, i.e.

The CPP guideline is clear: Function objects should be cheap to copy (and therefore passed by value).

but in practice that's not really true, as I believe with capture [*this] in lambdas, so before copying blindly, we may want to re-evaluate whether we are using correct patterns as that is not cheap to copy.

• The above mentioned rule doesn't consider that in general function objects, the operator() can be overloaded based on value category and cv-qualification of the callable object itself, and here perfect forwarding it matters. Consider:

struct CustomFunctor {
    void operator()() & {
        std::cout << "called on lvalue\n";
    }

    void operator()() && {
        std::cout << "called on rvalue\n";
    }
};

template <typename F>
void plain_invoke(F&& f) {
    f();  // f is a named variable => always an lvalue expression
}

int main(){
CustomFunctor f{};

plain_invoke(f); // correct behavior, output is "called on lvalue\n"
plain_invoke(CustomFunctor{}); // incorrect behavior, output "called on lvalue\n" and expected "called on rvalue\n"

return 0;
}

I just wanted to highlight a few points I came across lately for this discussion.

[TonyXiang8787]

Hi @figueroa1395

• std::ranges::viewable_range requires perfect forwarding (together with std::views::all) to work correctly with std::vector and std::span (with all their &, const, etc. versions).

It does not requires perfect forwarding to work correctly. It needs to be a universal reference, if the caller wants to pass an owning-mutable container, still no forward is needed. I will come back to this point later in the PR description.

NOTE: this is under the assumption that your function (and follow-up deeper calls) does not intend to take over the ownership of the resource.

We should not generalize to assume all function calls on ranges will potentially take over some ownership.

The CPP guideline is clear: Function objects should be cheap to copy (and therefore passed by value).

but in practice that's not really true, as I believe with capture [*this] in lambdas, so before copying blindly, we may want to re-evaluate whether we are using correct patterns as that is not cheap to copy.

• The above mentioned rule doesn't consider that in general function objects, the operator() can be overloaded based on value category and cv-qualification of the callable object itself, and here perfect forwarding it matters. Consider:

You are correct here. Generically, there can be a case where operator() has a rvalue reference overload. The question is: do we have this in our code base? As far as I know, not. I will address this too in the decription later.

The same goes with the cheap to copy functor. It's not that functors are always cheap to copy. But I think we can define cheap to copy, pass by value, as the coding rules of PGM core.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
97.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud