[Development] Views

[Lars Knoll]

> On 6 Jun 2019, at 14:49, Mutz, Marc via Development <development at qt-project.org> wrote:
> 
> On 2019-06-06 14:04, Lars Knoll wrote:
>>> On 6 Jun 2019, at 13:39, Mutz, Marc via Development <development at qt-project.org> wrote:
> [...]
>>> You are equating Qt users and Qt implementers. You can maintain the Qt API, but use more efficient data structures in the implementation. You seem to be implying that these two things cannot be separated.
>> The comment above was aimed at our APIs and towards our users, not
>> about how we implement things internally.
> 
> Ok, clash of context. This subthread was about the exact change in QtWayland, though, and that's internal implementation.
> 
>>> None of the changes where I replaced QMap changes the public API at all (except adding an overload because we can). No user is affected by this in any way, except that they have a few pages of memory more free than before.
>>> Please explain to me how any of those changes makes _users_ of Qt have to revert to the STL?
>>> And please explain to me how it can possibly be worthwhile to generate 8KiB of code _just_ to not have to use lower_bound? Which argument could *possibly* be made against a lower_bound? Esp. seeing as many attempts to write one by hand have failed. I remember a bug about shortcuts being mapped to the wrong key, because the hand-rolled binary search was unstable.
>> You have in general been advocating lately to remove/deprecate lots of
>> Qt API in favour of STL. This is what I referred to, not
>> implementation details in our code.
> 
> Ok, noted.
> 
>> Removing the usage of a QMap for a mapping of 5 values is the right
>> thing to do. Whether we need lower_bound() for that and a sorted list,
>> or simply an unsorted vector where we iterate until we find the right
>> value doesn’t matter too much in this case. Performance wise both
>> would probably be equivalent :)
>>> I'm sorry, but we have a lot of code that is less readable than any of the changes I uploaded. It just cannot be an argument to say that it's unreadable because it uses an STL algorithm. This sentiment has caused so very, very many quadratic loops because people get the impression that std::remove_if is toxic, and in each one the solution was to use remove_if, because the hand-rolled alternative would be totally unreadable.
>> Nobody here said that. I also believe those changes that you proposed
>> got approved to the largest degree.
> 
> This is not the impression I get from https://codereview.qt-project.org/c/qt/qtwayland/+/264069, or https://codereview.qt-project.org/c/qt/qtbase/+/264129 vs. https://codereview.qt-project.org/c/qt/qtbase/+/264161
> 
>> If you read what I said earlier, I also said that in most cases better
>> data structures and algorithms will lead to more readable code. I am
>> not opposed to using those in Qt, quite the opposite.
> 
> Good.
> 
>>> I'm sad to see that Qt devs think so lowly of themselves as to be unable to understand a piece of code that uses an STL algorithm. Really. I'm out of words.
>> Nobody ever said that as far as I can tell. What people said is that
>> there are cases, where a loop is just as efficient and easier to read
>> (e.g. when searching for an entry). And in that case it’s fine to
>> simply use that. So please stop turning the meaning of what people
>> said around like that.
> 
> I was a poignant remark, yes, but that's _exactly_ the feedback I get from the reviews I quoted above: Dropping QMap is acceptable when the API is unchanged (as with the array replacing the map), but totally not acceptable (b/c/o unreadable) if it contains lower_bound or find_if. To the point that the 'better' choice is to replace QMap with QHash - because they have the same API. I'd ROTFL if I wasn't crying.

The discussion showed that replacing QMap with QHash gives you most of the improvement that you wanted.

I can understand that maintainers don’t like if the change adds ~40 lines that can’t be shared, and complicate the code. We are spending a lot of time maintaining our code base, and simplifying our lives there is extremely important as we have limited man-power available. This is especially true if similar changes are being done in many places in our code base.

With respect to that I can see why people argue for using either a QHash (where the change is a one liner), or ask you to consider providing a (maybe internal) special class that can be used as a drop in replacement.

Cheers,
Lars