https://bugs.kde.org/show_bug.cgi?id=507547
--- Comment #12 from Mladen Milinkovic, Max <[email protected]> --- (In reply to Matthias Grimrath from comment #11) > You dare to go, were I wouldn't, by using allocators :D > > This, however, looks suspicious to me > > inline bool inContainer() { > const auto &a = container()->get_allocator(); > return this >= a.lastData && this < a.lastData + a.lastSize; > > so the pointer is compared against the allocated memory. However, a pointer > somewhere into that allocated memory may not point to an actually used > element. We saw QVector<> reserving some space at the beginning to be able > to quickly insert elements at the top. And a STL implementation of > std::vector<> is probably allowed to do the same. As long as memory is allocated to that container it's fine IMHO, regardless if it's reserved or used. If something is moving object into memory allocated by m_lines it's about to become or already is member of m_lines. > I don't know weather that leads to real-world problems. But I feel uneasy > about it. > > I think a better approach would be to control insertion of new elements into > 'm_lines' and update indices there, instead of hooking into QVector<> or > std::vector<> by template magic. Should be the same performance wise. It's not magic - just code :D - and this one doesn't rely on unspecified internal implementation of std::vector. Previous version relied on QVector constructing elements in memory between begin() and end() (it had to update begin/end() *before* move/copy/constructing element object). If you want to do PR it's fine by me - although keep in mind that similar approach was used before and it wasn't working well. At times when subtitle or undo stack handling code was touched it often caused subtle bugs and it was maintenance nightmare. Sorting performance was awful due to index recalculation every time lines would get swapped and so on. :) > I'll work on a patch, having real code to show is probably better than > sophisticated arguments :D The safest (code wise) and best (performance wise) would be to use custom array class and handle memory allocation directly, but I'm quite happy with custom allocator like it is now. -- You are receiving this mail because: You are watching all bug changes.
