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

Ranges require exception to F.19 #2157

Open
adah1972 opened this issue Nov 2, 2023 · 7 comments
Open

Ranges require exception to F.19 #2157

adah1972 opened this issue Nov 2, 2023 · 7 comments

Comments

@adah1972
Copy link
Contributor

adah1972 commented Nov 2, 2023

I noticed F.19: For “forward” parameters, pass by TP&& and only std::forward the parameter when my code triggered cppcoreguidelines-missing-std-forward in clang-tidy.

As Nicolai Josuttis and Arthur O'Dwyer mentioned, “If you see code using (deduced) T&& without std::forward<T>, it’s either buggy or it’s C++20 Ranges.” Yes, ranges code often requires a forwarding reference without needing to forward it. The range object may be modified during the iteration, so we cannot capture it with const T&. It can be an rvalue, so we cannot use T& either. Only T&& is viable. However, forwarding it often does no good at all, as shown by the example below:

#include <iostream>
#include <ranges>
#include <vector>

template <typename Rng>
void Access(Rng&& rng)
{
    for (const auto& item : rng) {
        std::cout << item << '\n';
    }
}

int main()
{
    std::vector vec{1, 2, 3, 4, 5, 6};
    Access(vec | std::views::filter([](int n) { return n % 2 == 0; }));
}

Forwarding rng (anyone wants to write for (const auto& item : std::forward<Rng>(rng))?) is weird, useless, and unnecessary. Doing so would probably cause more confusion: Why forwarding it here? What good does it do?

So I believe an exception should be added to F.19.

@adah1972
Copy link
Contributor Author

adah1972 commented Nov 5, 2023

If people agree to the changes, I can put down words explaining this issue.

The enforcement part is more complicated, but still doable. If the TP&& parameter is not forwarded but begin and end is called on this parameter (standalone or member functions), including implicitly in the range for, then this should be regarded as the exception case and should not be flagged as a violation.

@Quuxplusone
Copy link
Contributor

I don't personally care what the Core Guidelines say on this topic, but FWIW, if I had to write an enforcement check for this, I'd say:

  • A non-parameter must not be forwarded. (Use static_cast or std::move, not std::forward.) [1]
  • A parameter that's not taken by forwarding reference must not be forwarded. [1]
  • A parameter constrained to be std::ranges::range-or-better must be taken by forwarding reference, and it should not be std::forward'ed (without some special documentation/exception to the rule).
  • A non-range parameter that is taken by forwarding reference should be forwarded.

[1] - https://quuxplusone.github.io/blog/2023/05/27/dont-forward-non-forwarding-references/

@geza-herman
Copy link

How predicates would be passed in the STL, if the STL was designed today? Isn't it possible that we'd use forwarding references to pass them? I think that predicates are similar to ranges, as we don't expect a move operation on them. If an algorithm uses a predicate, there is no "sink", there won't be a move operation, even if the predicate is an rvalue.

I can think of other cases where I'd use forwarding references (for the same reason that is described here, to avoid having multiple overloads of a function), but then wouldn't use std::forward on them. It is not just ranges that have this property.

That's why that at the moment, I like the original "universal references" name better, because it just tells what it is and not how it is supposed to be used.

@Quuxplusone
Copy link
Contributor

How predicates would be passed in the STL, if the STL was designed today? Isn't it possible that we'd use forwarding references to pass them?

STL predicates must be const-callable, and (as you said) never need to be copied, so they'd be passed by const&.
Some implementations (notably libc++) already do this internally, although they still pass by-value at the very topmost level because that's what the Standard says to do.

Isn't it possible that we'd use forwarding references to pass them?

Perhaps, but then we'd have to std::forward them when we passed them onward; otherwise, their original value category (lvalue or rvalue) would be lost. Contrariwise, if we don't care about their value category, we'd just take them by const& (and not forward them).

@geza-herman
Copy link

How predicates would be passed in the STL, if the STL was designed today? Isn't it possible that we'd use forwarding references to pass them?

STL predicates must be const-callable, and (as you said) never need to be copied, so they'd be passed by const&. Some implementations (notably libc++) already do this internally, although they still pass by-value at the very topmost level because that's what the Standard says to do.

But what if STL predicates designed to be non-const-callable? Today, with the tool of forwarding reference, maybe predicates were defined differently. Or, if we just don't consider standard compliant predicates. I'm free to design a system, were predicates have less constraints, right? And then it would make sense to use forwarding references for this kind of predicate.

Isn't it possible that we'd use forwarding references to pass them?

Perhaps, but then we'd have to std::forward them when we passed them onward; otherwise, their original value category (lvalue or rvalue) would be lost. Contrariwise, if we don't care about their value category, we'd just take them by const& (and not forward them).

But then we'd face the same problem that ranges had. With my hypothetical predicate, I don't care too much about the value category (just like ranges don't care about the value category either, as far as I understand), so std::forward is not needed. But I care about const, just like ranges. An algorithm function which receives an rvalue predicate doesn't receive an rvalue because the caller cares about forwarding/moving, but it was just more convenient to pass a temporary to the function.

Let's see a find_if example. Suppose that it has a forwarding reference predicate. Should (or even, can) find_if use std::forward for the predicate? I think the answer is no. In the search loop, it should not use std::forward, because then the predicate may be moved out of multiple times. If we want to stick to strict semantics, then we should call std::forward at the last iteration. But we don't know which will be the last iteration, because it depends on the return value of the predicate. So it doesn't make too much sense that find_if tries to keep the value category. But it still a good thing that find_if can be called with any value-category predicate, hence the predicate is a forwarding reference. So in the end, we could just say, that for predicates, moving doesn't make any sense (at least, not how the standard library uses them), therefore it doesn't make any sense that the standard library tries to forward them. But of course, the user may still implement moving ctor/assignment and allowed to use it in their code, it's just the standard library which doesn't use it.

But I can give you another (more complicated) example if this is not convincing enough. Let me know if you're interested!

@Quuxplusone
Copy link
Contributor

@geza-herman wrote:

But what if STL predicates designed to be non-const-callable?

Then they wouldn't be predicates. "Predicate" is a term with specific connotations. If you were designing something that didn't act that specific way, then you wouldn't call it a "predicate"; you'd call it something else.

With my hypothetical predicate, I don't care too much about the value category (just like ranges don't care about the value category either, as far as I understand), so std::forward is not needed. But I care about const, just like ranges.

Sure, if you design something (not a "predicate", but something else) that uses the same broken understanding of rvalueness that Ranges uses — that is, if you use "is an rvalue" to signal "is short-lived and will dangle" instead of to signal "is expiring and can be moved from" — then you'll have all the same problems, with all the same workarounds, that Ranges has, and you'll end up with the same rules of thumb. Your thing (whatever it is) will be always passed by forwarding reference and never forwarded.

Going back to your first comment: Your original hypothetical was basically "What if there were some kind of animal, not a range, that behaved like ranges do? Wouldn't we also pass that kind of animal by forwarding reference?" So far so good. Your comment went off the rails only when you tried to denote that new kind of animal using the word "predicate" (which is already a meaningful technical term that absolutely does not mean that kind of animal). If you had said instead...

How gostaks would be passed in the STL, if the STL was designed today? Isn't it possible that we'd use forwarding references to pass them? I think that gostaks are similar to ranges, as we don't expect a move operation on them. If an algorithm uses a gostak, there is no "sink", there won't be a move operation, even if the gostak is an rvalue.

...then I would have had no problem with your hypothetical at all. Yes, in that case, one would want to modify my rules of thumb to say

  • A parameter constrained to be std::ranges::range-or-better, or a parameter that is a gostak, must be taken by forwarding reference, and it should not be std::forward'ed.
  • A non-range non-gostak parameter that is taken by forwarding reference should be forwarded.

My rules work only because ranges are the only animals in present-day C++ that operate by idiosyncratic rvalueness rules. If new such animals appear, new rules must appear too.

@geza-herman
Copy link

broken understanding of rvalueness that Ranges uses

I wouldn't call this understanding broken. C++ gives us a feature, called forwarding reference. It has some rules how it binds to objects. Most of the time, this reference is forwarded. But not always. That's why I like the original "universal reference" better than the current "forwarding reference". It tells what it is and not how it is supposed to be used. We don't call lvalue references as "output-parameter references", or const rvalue references as "input-parameter references" (exaggerated analogy, I know). I understand this new kind of reference is developed to solve the forwarding problem. But it can be used in different ways as the design of ranges proves.

if you use "is an rvalue" to signal "is short-lived and will dangle" instead of to signal "is expiring and can be moved from"

"Signal" may not be the best way to describe this. I just want to create a function, which can have a parameter with any value category. And this parameter is not supposed to be moved inside the function (just like ranges). C++ gives me forwarding references to do this. I understand that it is against the idea "forwarding references should be forwarded". But nothing tells me, for my use case, that this should be the case.

My rules work only because ranges are the only animals in present-day C++ that operate by idiosyncratic rvalueness rules. If new such animals appear, new rules must appear too.

It is true for the C++ standard library, but not for C++ in general. If people create new libraries/systems, these libraries can have functions which have the same idiosyncratic rules. Just to be clear, I agree with your rules if we only consider the standard library. The new animals are already here, it's just they are not in the standard library. I think CppCoreGuidelines is not constrained to the usage of the standard library. I mean, if people consider F.19, they might think that it's a mistake to design something like the range system, because it doesn't follow F.19.

Here's an example, which I believe is a fine design, yet has the same idiosyncrasy. Suppose that I want to create some kind of visitor which visits a type (can be used in a reflection system):

template <typename TYPE, typename VISITOR>
void visit(VISITOR &&visitor);

If TYPE is a primitive type, visit calls visitor.on_primitive<...>(...). If it is an array, it calls visitor.on_array<...>(...). If it is a class, it calls these in order: visitor.on_begin<...>(...), visitor.on_member<...>(...) for each member of the class, and finally visitor.on_end<...>(...). Now, visit doesn't care about the value category of visitor, it just calls its member functions (even, from the perspective of visit, moving the visitor doesn't make any sense). VISITOR can use different design ideas: it may contain some container, that will be filled by visit. In this case, visitor is passed as an lvalue reference: SchemeVisitor scheme; visit<MyType>(scheme); (here, visit fills scheme to contain some description of MyType). Or, VISITOR may be just a forwarding thing, so it only contains a pointer, and the user of visit may pass it as an rvalue: std::vector<unsigned char> serialized_scheme; visit<MyType>(SchemeSerializerAdapter(&serialized_scheme)) (here, visit fills serialized_scheme with some serialized version of the type of MyType). I think nothing is broken/buggy here, yet visitor is a forwarding reference, but it is not forwarded inside visit.

The problem is, CppCoreGuidelines doesn't contain a rule for this use case. The title of F.19 says “For ‘forward’ parameters”, so in a sense, it doesn't apply to my visitor, because semantically it is not forwarding. But this is true for the original issue as well: ranges also don't forward ranges, it just uses forwarding reference. So, it may not covered by F.19. But the problem is, if F.19 is used in linters, the linter doesn't have any chance to differentiate between this idiosyncratic use case, and the "proper" forwarding case. But as F.19 says "A parameter of type TP&& should essentially always be passed onward via std::forward in the body of the function.", a linter may warn for this idiosyncratic case (and as a matter of fact, clang-tidy has cppcoreguidelines-missing-std-forward which warns for my visit).

The only reason I added my original comment is to say that it's not just only ranges which may need an exception from F.19. Which may be disregarded of course, as this idiosyncratic use case is rare, we can flag such functions with NOLINT.

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

3 participants