[Development] Rvalue pinning in Qt

[Rafael Roquetto]

Hi,

On 6/21/22 11:26, Giuseppe D'Angelo via Development wrote:
> Hi,
> 
> On 20/06/2022 17:39, Thiago Macieira wrote:
>> On Monday, 20 June 2022 07:46:57 PDT Giuseppe D'Angelo via Development 
>> wrote:
>>> A fancy name for: "if a function/class is operating on a rvalue, should
>>> it store a copy of it in order to keep it alive?". Consider
>> In other words, remove the rvalue reference and store a copy from the 
>> const-
>> lvalue reference overload you already have.
> 
> Well, not necessarily. Say the input is a lvalue std::u16string; you 
> don't want the tokenizer to unconditionally copy it.
> So it just takes a reference, and should continue to do so.

I'll just give my two cents here (and sorry if I end up re-stating the 
obvious):

  1. taking an rvalue can dangle, and robust code should not accept that
  2. always copying can be potentially expensive.

Point number 1 trumps point number 2 IMHO, as it ensures that code will 
never break, at the expense of a performance penalty in _relevant 
hotpaths_. I would solve that by providing the user with a non-owning 
separate API analogous to QByteArray::fromRawData(), that operates on 
lvalue references.

This makes the reasoning over the API simpler IMHO - the base case is 
robust, always working as intended. The user is free to pass the object 
around as it's self-contained.

Should performance be an issue, the user can resort to the non-owning 
version. This forces the user to reason about the code rather than 
blindly and inconsequentially relying on some overload and spares a 
future reader from having to figure out solely from context if a given 
statement is copying or simply referencing a value.

To piggy-back on your example (Haystack&& represents an rvalue, not 
universal refs, pseudo-code for illustration purposes):

1. QStringTokenizer(Haystack h, ...); // always copy
2. QStringTokenizer::makeView(const Haystack&, ...); // always reference 
(only lvalues)
3. QStringTokenizer::makeView(Haystack&&, ...); // disabled/deleted - 
compile error

(1) should be enough for most use-cases. (2) opens up for the 
possibility of having a tokenizer that potentially carries a dangling 
reference, but should only be used in optimization paths or local 
scopes. (3) emits a compile error (the user should probably be using 1 
in this case).

Not sure how scalable this approach is in terms of a global Qt API, though.

- Rafael

> 
> (Looking at std::views::all: this is the case where the input is a 
> non-view lvalue, so you'll wrap it in ref_view -- i.e. merely hold a 
> reference to it.)
> 
> 
>>>> std::u16string someLongString = u"...";
>>>> auto tokenizer = QStringTokenizer(someLongString, u"X");
>>> For this reason QStringTokenizer moves and stores the input if it's an
>>> rvalue, but only keeps a reference if it's an lvalue.
>> This means QStringTokenizer must have a QString member and a QString & 
>> member.
>> The simplest implementation removes one of them.
> 
> Not always, see above.
> 
> 
>>>> QString s = getString();
>>>> auto tok = QStringTokenizer(s, u"x");
>>> would take a copy (given `s` is a view after [4]), while now it only
>>> takes a reference.
>> Yup.
>>
>>> Opinions?
>> My only objection is to calling this by a fancy name, "rvalue 
>> pinning". Simply
>> call what it is: take all parameters by const-lvalue and never store a
>> reference.
> 
> It's also imprecise. We should use the C++20 range terminology which is 
> more accurate, but I don't want to cause even more confusion (given we 
> don't use that logic, and probably _can't_ use the logic just yet).
> 
> 
> Thanks,
> 
> _______________________________________________
> Development mailing list
> Development at qt-project.org
> https://lists.qt-project.org/listinfo/development