Repository: microsoft/terminal
File: src/buffer/out/textBuffer.cpp (C++)

Code before:
            for (const auto str = i->str(); const auto& glyph : til::utf16_iterator{ str })
            {
                matchSize += IsGlyphFullWidth(glyph) ? 2 : 1;
            }
            const auto end = start + matchSize;
            lenUpToThis = end;

            const til::point startCoord{ start % rowSize, start / rowSize };
            const til::point endCoord{ end % rowSize, end / rowSize };

            // store the intervals
            // NOTE: these intervals are relative to the VIEWPORT not the buffer
            // Keeping these relative to the viewport for now because its the renderer
            // that actually uses these locations and the renderer works relative to
            // the viewport
            intervals.push_back(PointTree::interval(startCoord, endCoord, idAndPattern.first));
        }
    }
    PointTree result(std::move(intervals));
    return result;
}

std::vector<ScrollMark>& TextBuffer::GetMarks()
{
    return _marks;
}
const std::vector<ScrollMark>& TextBuffer::GetMarks() const
{
    return _marks;
}

// Remove all marks between `start` & `end`, inclusive.
void TextBuffer::ClearMarksInRange(
    const til::point start,
    const til::point end)
{
    auto inRange = [&start, &end](const ScrollMark& m) {
        return (m.start >= start && m.start <= end) ||
               (m.end >= start && m.end <= end);
    };

    _marks.erase(std::remove_if(_marks.begin(),
                                _marks.end(),
                                inRange),
                 _marks.end());
}
void TextBuffer::ClearAllMarks() noexcept
{
    _marks.clear();
}


Review comment (bug):
I don't think this function is a good idea.

For instance, it leaks the way marks are stored. A `vector` is not a good way to store them and a ring buffer would be a better choice, because it makes the 2 most common operations much cheaper (adding at the end, removing at the beginning during buffer circling). Changing the underlying storage might require changing all callers around.
More importantly however I believe it's more bug-prone, because it allows the caller to make modifications that `TextBuffer` doesn't know about. Like, let's say we store marks sorted by start coordinate in the future to make range-based access and removal cheaper (`n` -> `log n`). If I change the `start` coordinate outside of `TextBuffer` then, this might break the sort order and cause the range-based functions to fail in weird ways. This might be obvious now, but I'm sure `COOKED_READ_DATA` was also obvious to someone at some point and now look at it (and us maintaining it). 😄 It has the exact same design flaw. It leaks its internals (and so does `GetRowByOffset` but I'm slowly working on fixing that).

Code after:
            for (const auto str = i->str(); const auto& glyph : til::utf16_iterator{ str })
            {
                matchSize += IsGlyphFullWidth(glyph) ? 2 : 1;
            }
            const auto end = start + matchSize;
            lenUpToThis = end;

            const til::point startCoord{ start % rowSize, start / rowSize };
            const til::point endCoord{ end % rowSize, end / rowSize };

            // store the intervals
            // NOTE: these intervals are relative to the VIEWPORT not the buffer
            // Keeping these relative to the viewport for now because its the renderer
            // that actually uses these locations and the renderer works relative to
            // the viewport
            intervals.push_back(PointTree::interval(startCoord, endCoord, idAndPattern.first));
        }
    }
    PointTree result(std::move(intervals));
    return result;
}

const std::vector<ScrollMark>& TextBuffer::GetMarks() const noexcept
{
    return _marks;
}

// Remove all marks between `start` & `end`, inclusive.
void TextBuffer::ClearMarksInRange(
    const til::point start,
    const til::point end)
{
    auto inRange = [&start, &end](const ScrollMark& m) {
        return (m.start >= start && m.start <= end) ||
               (m.end >= start && m.end <= end);
    };

    _marks.erase(std::remove_if(_marks.begin(),
                                _marks.end(),
                                inRange),
                 _marks.end());
}
void TextBuffer::ClearAllMarks() noexcept
{
    _marks.clear();
}

// Adjust all the marks in the y-direction by `delta`. Positive values move the
// marks down (the positive y direction). Negative values move up. This will
// trim marks that are no longer have a start in the bounds of the buffer
void TextBuffer::ScrollMarks(const int delta)