[Development] Important recent changes in QList/QString/QByteArray

[Andrei Golubev]

Hello,

Let me try to improve the wording from the original mail and clarify certain things. Sorry for not being pedantic enough at the beginning.

"Non-detaching non-const modifying operations" mean non-detaching non-const member functions of the QCCs*, sorry for ambiguation.

*Qt Contiguous Container
(Thanks for the abbreviation, by the way).


Now, let's distinguish between non-const member functions:

  *   Mutating - affect the number of elements in container or space arrangement:
     *   Number of elements: erase, insert, append, prepend (also operator overloads - e.g. operator+=), etc.
     *   Space arrangement: squeeze, resize, reserve
  *   Non-mutating - still non-const but do not change the number of elements or space arrangement:
     *   Data accessors: begin(), end(), data(), operator[], at() and so on

With the difference in mind, more precise definition of "non-detaching non-const modifying operation" is any mutating member function of the QCC.
So, any mutating member function of QCC can invalidate iterators/pointers/references to elements.
For non-mutating member functions pre-existing requirements should hold without changes (at least from the point of prepend optimization and caused disruptions), for instance, begin() may cause detach and invalidate your iterators.
So, now, formally, std::sort(v.begin(), v.end()) risks undefined
behavior? E.g. begin() returns the begin iterator without touching
anything, but end() decides to invalidate all the iterators.
This should be a defined behavior regardless of whether v needs to be detached or not. If it's suddenly UB, it's a bug in the QCC. (In detach case, we would detach before returning begin/end so this should also be safe or am I wrong?)

However, doing something like the following is UB:

auto begin = v.begin(); auto end = b.end();
std::sort(begin, end);
v.copyAppend(42);   // assume 42 is of suitable type and can be appended to v
std::sort(begin, end);  // UB, since copyAppend() _may have invalidated_ the iterators

Note: the case is UB even when there is enough capacity to store 42 without reallocation, this is an example of what changed. Yet, from what I see, there were no guarantees in documentation beforehand, so most likely:
Your code was incorrect in the past, it just happened to work.

Now, this "_may have invalidated_" depends a lot on a) specific member function called; b) the way QCC is filled with data; c) previous modifying (member) operations done on the QCC and this is probably not an exhaustive list of things to consider.
Since the behavior of invalidation is rather non-deterministic, at this moment it is easier (for library developers, not for users) to consider the invalidation to happen always. Any exceptions or cases in which iterators/pointers/references would stay valid should be documented explicitly.

As for Guiseppe's question:
This is the contract, and it's OK. I don't think however that this is
what was intended by OP. Rather, that calling a non-const function may
repack/reallocate a QCC (*), causing invalidation of all references and
iterators, even if the container was NOT shared in the first place.

Did I misunderstand the problem?
If the function called is a mutating member function, then it causes the invalidation of all references and iterators, yes. It is correct (and only correct at this stage) to assume that it happens _always_ for any mutating member function.

Ville:
Interesting. I'm curious what sort of repacking happens on erase
The iterators before or after may be invalidated. Exactly which of the two (before or after) is going to happen depends on the position of the to-be-erased range with regards to the position of the full range.
the element storage wouldn't necessarily be at the beginning of the
allocated block;
in that approach, a pop_front would merely bump the begin, and erase
still wouldn't
invalidate anything before the erased position.
I guess you mean "erase still wouldn't invalidate anything after the erased position".  But this is effectively undefined (or implementation-specific, as you wish) and may change in future if we discover a more performing strategy. Again, any exceptions should be documented. Please do not assume any specific behavior otherwise.

--
Best Regards,
Andrei
________________________________
From: Development <development-bounces at qt-project.org> on behalf of Ville Voutilainen <ville.voutilainen at gmail.com>
Sent: Wednesday, September 2, 2020 11:04 AM
To: Giuseppe D'Angelo <giuseppe.dangelo at kdab.com>
Cc: Qt development mailing list <development at qt-project.org>
Subject: Re: [Development] Important recent changes in QList/QString/QByteArray

On Tue, 1 Sep 2020 at 20:44, Giuseppe D'Angelo via Development
<development at qt-project.org> wrote:
>
> Il 01/09/20 19:33, Thiago Macieira ha scritto:
> > All non-const functions that may detach should be coded so they DO detach.
> > That is, after any and all non-const functions, the refcount of the container
> > should be 1.
>
> This is the contract, and it's OK. I don't think however that this is
> what was intended by OP. Rather, that calling a non-const function may
> repack/reallocate a QCC (*), causing invalidation of all references and
> iterators, even if the container was NOT shared in the first place.
>
> Did I misunderstand the problem?

Interesting. I'm curious what sort of repacking happens on erase, and why
it wasn't done in such a way that e.g. QVector is 4 pointers instead
of 3, so that
the element storage wouldn't necessarily be at the beginning of the
allocated block;
in that approach, a pop_front would merely bump the begin, and erase
still wouldn't
invalidate anything before the erased position.
_______________________________________________
Development mailing list
Development at qt-project.org
https://lists.qt-project.org/listinfo/development
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20200902/8e993baf/attachment-0001.html>