<xstring>: Potentially confusing comment in begin()

[StephanTLavavej]

See 92508be#r35109869 . I believe that this comment could be easily clarified to explain that const Meow& binds to both lvalues and rvalues.

[CaseyCarter]

To me, this suggestion crosses the line from describing what the library is doing into explaining how the language works. I don't think we want to be in the business of describing the core language in our comments.

[miscco]

I would second that, however misleading comments do not help either. Omitting it seems way better
// non-member overload to model the exposition-only forwarding-range concept

[StephanTLavavej]

While I generally agree with @CaseyCarter (we assume a high level of familiarity with Core), this really looks like an lvalue/rvalue confusion at first glance. I like @miscco ‘s new suggestion of shortening the comment, what do you think?

(We also do describe Core behavior when it is very intricate, e.g. the default-init/value-init comment in charconv. The const T& thing is basic, it’s just the typo-looking nature that persuades me.)

[timsong-cpp]

const basic_string_view& loses to the poison pill T&& on rvalues, so I don't think it's even right.

[CaseyCarter]

const basic_string_view& loses to the poison pill T&& on rvalues, so I don't think it's even right.

Ah, thanks - something has been bothering me the entire time we've been talking about this issue and you've put your finger on it. I have a compiler bug to reduce.

[brevzin]

@CaseyCarter, I was intending on using this as further justification for this opt-in mechanism being too subtle: https://brevzin.github.io/cpp_proposals/1870_forwarding_range/d1870r0.html#public-shaming

[CaseyCarter]

Test case (https://godbolt.org/z/wHdmsb):

namespace detail {
    // Avoid MSVC bug when only deleted functions are found in phase 1 name lookup
    void begin();

    template <class T>
    void begin(T&&) = delete;

    template <class T>
    concept has_ADL = requires(T&& t) {
        begin(static_cast<T&&>(t));
    };
}

template<class T>
struct basic_string_view {
    friend T const* begin(const basic_string_view&) {
        return nullptr;
    }
};
using string_view = basic_string_view<char>;

static_assert(!detail::has_ADL<::string_view>);

For the unqualified call to begin in has_ADL with an xvalue string_view, both the deleted template begin (the so-called "poison pill") and the non-member friend defined in basic_string_view are candidates. The implicit conversion sequences from xvalue string_view to string_view&& and to const string_view& are both identity conversions since the reference binds directly ([[over.ics.ref]/1). The call is nevertheless unambiguous due to the tie-breaker rule in [over.ics.rank]/3.2:

(3.2) Standard conversion sequence S1 is a better conversion sequence than standard conversion sequence S2 if

[...]
(3.2.3) — S1 and S2 include reference bindings ([dcl.init.ref]) and neither refers to an implicit object parameter of a non-static member function declared without a ref-qualifier, and S1 binds an rvalue reference to an rvalue and S2 binds an lvalue reference

Since the poison pill binds an rvalue reference to the xvalue argument, it has a better conversion sequence than the non-member friend that binds an lvalue reference to that argument.

EDIT: Reported at Developer Community.

@CaseyCarter, I was intending on using this as further justification for this opt-in mechanism being too subtle

Feel free: this is easily the second-most problematic opt-in mechanism in the Ranges design. I should point out that it's not purely an opt-in: the range access customization point objects (CPOs) actually do call these rvalue begin and end overloads with rvalues, but it would simplify the implementation of the CPOs and possibly provide a minor throughput improvement if we did not. The CPOs always have access to an lvalue (their reference parameter) even when the argument is an rvalue, there's no reason they could not if constexpr (argument_is_an_lvalue || argument_type_opts_in_to_rvalue_magic) { just_do_the_lvalue_thing(); } and instantiate fewer function templates overall.

[CaseyCarter]

I would second that, however misleading comments do not help either. Omitting it seems way better
// non-member overload to model the exposition-only forwarding-range concept

@miscco FWIW, I don't think the original comment was misleading - accepting rvalues is the crucial factor necessary to model the concept - but I'm happy to make this change along with the actual bugfix.

[StephanTLavavej]

@CaseyCarter , if you’re planning to make an STL code change along with a comment change here, this should probably be labeled as bug instead of (or in addition to) documentation. (I am unsure whether you are blocked by the C1XX bug, or if a workaround is possible. If blocked, we should probably have a label.)

[miscco]

@CaseyCarter I absolutely agree that mentioning rvalues is essential here. With the changed code it will also make perfect sense. However, I would suggest to modify it to explicit mention the opt-in

ifdef __cpp_lib_concepts
    // non-member overloads that accept rvalues to opt-in the exposition-only forwarding-range concept
    _NODISCARD friend constexpr const_iterator begin(basic_string_view&& _Right) noexcept {
        return _Right.begin();
    }
    _NODISCARD friend constexpr const_iterator end(basic_string_view&& _Right) noexcept {
        return _Right.end();
    }
#endif // __cpp_lib_concepts

Although reading it the aloud makes the that accept rvalues feel unneeded. Wording is hard...