fix: take indices to messages as a hint

[Nerixyz]

2804 shows that using indices across different channels can be buggy. In ChannelView we maintain two buffers - the buffer for the virtual channel and the buffer for the messages. But more importantly, we blindly re-use indices from messageReplaced events from the underlying channel to index into the virtual channel. Both channels don't necessarily have the same messages (e.g. when using filters). Thus, we can't re-use the indices. However, as time shows, in most cases, the index we get will be correct, as most of the time, that index is zero (i.e. no messages have been added since).

This PR adds a "middle-way" to solve this. LimitedQueue can now take a hint to find and replace items. If that hint points to the expected item, no additional work is required. However, if that hint doesn't match, the whole buffer is searched for the desired item.

Fixes #2804 (hopefully, I couldn't replicate it - if this doesn't solve it, we have reentrancy issues when replacing messages)

[hemirt]

if this doesn't solve it, we have reentrancy issues when replacing messages

By this, do you mean the fact that the circular buffer could have shifted underneath?

That could have also caused this, that was what I had in mind looking at this, and thought of a similar solution to yours (i.e. replace index with the message pointer and check based on that).

Other than that I didnt quite think up any other possible causes.

Of course, supposing this was the problem, I tried forcing replication and still didn't achieve much, though maybe a hardcoded test with this could replicate it.

I think your tests do not test when the buffer is full - it might be something to look at.

[hemirt]

(a side note) https://github.com/Chatterino/chatterino2/pull/5683/commits/6430682902134de4c7d4adb199daff3ae1e95eb5 ULL suffix should be in c++11, i did not see you use the c++23 z suffix. (If needed there's also the possibility of using custom suffixes - user literals)

(Maybe im just seeing sth wrong in github)

[Nerixyz]

By this, do you mean the fact that the circular buffer could have shifted underneath?

Yes. Probably a bit more general than reentrancy, but related in that assumptions about indices are invalidated.

Of course, supposing this was the problem, I tried forcing replication and still didn't achieve much, though maybe a hardcoded test with this could replicate it.

I could test Channel, but I can't test ChannelView (which is where the bug seems to be).

I think your tests do not test when the buffer is full - it might be something to look at.

Sure.

(a side note) 6430682 ULL suffix should be in c++11, i did not see you use the c++23 z suffix. (If needed there's also the possibility of using custom suffixes - user literals)

(Maybe im just seeing sth wrong in github)

ULL isn't portable. On Windows, ULL is equivalent to a size_t, but on other platforms, where sizeof(int) != sizeof(long), UL is equivalent to size_t. Adding a custom suffix for this test would be too much imo (maybe in a refactor).

[hemirt]

ULL isn't portable. On Windows, ULL is equivalent to a size_t, but on other platforms, where sizeof(int) != sizeof(long), UL is equivalent to size_t. Adding a custom suffix for this test would be too much imo (maybe in a refactor).

Didn't know that there were some things like that with regards to size_t type, granted, unless it is larger than unsigned long long, which the verbiage in standard seems to allow, using ull should be fine to cover possible issues with regards integer conversion and comparison problems.

I didn't know the underlying issue and just saw the commit name and the code, so thought something didnt add up, thanks for the explanation.

But yeah, this isnt really much needed in the tests 👍

[Nerixyz]

should be fine to cover possible issues with regards integer conversion and comparison problems.

If you only have an argument of size_t it's usually fine. But if you have it as a template argument, this breaks: https://godbolt.org/z/3vGE389x4.