Repository: microsoft/terminal
File: src/renderer/vt/VtSequences.cpp (C++)

Code before:
                        ((WI_IsFlagSet(wAttr, FOREGROUND_INTENSITY)) ? 60 : 0) +
                        (WI_IsFlagSet(wAttr, FOREGROUND_RED) ? 1 : 0) +
                        (WI_IsFlagSet(wAttr, FOREGROUND_GREEN) ? 2 : 0) +
                        (WI_IsFlagSet(wAttr, FOREGROUND_BLUE) ? 4 : 0);

    // An example string with max length would be "\x1b[100m", which has length = 6.
    char buf[6];
    char* end = fmt::format_to(std::begin(buf), FMT_COMPILE("\x1b[{}m"), vtIndex);
    return _Write({ buf, static_cast<std::string_view::size_type>(end - buf) });
}

// Method Description:
// - Formats and writes a sequence to change the current text attributes to an
//      indexed color from the 256-color table.
// Arguments:
// - wAttr: Windows color table index to emit as a VT sequence
// - fIsForeground: true if we should emit the foreground sequence, false for background
// Return Value:
// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
[[nodiscard]] HRESULT VtEngine::_SetGraphicsRendition256Color(const WORD index,
                                                              const bool fIsForeground) noexcept
{
    // An example string with max length would be "\x1b[38;5;128m", which has length = 11.
    char buf[11];
    char* end = fmt::format_to(std::begin(buf), FMT_COMPILE("\x1b[{};5;{}m"), fIsForeground ? 38 : 48, ::Xterm256ToWindowsIndex(index));
    return _Write({ buf, static_cast<std::string_view::size_type>(end - buf) });
}

// Method Description:
// - Formats and writes a sequence to change the current text attributes to an
//      RGB color.
// Arguments:
// - color: The color to emit a VT sequence for
// - fIsForeground: true if we should emit the foreground sequence, false for background
// Return Value:
// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
[[nodiscard]] HRESULT VtEngine::_SetGraphicsRenditionRGBColor(const COLORREF color,
                                                              const bool fIsForeground) noexcept
{
    DWORD const r = GetRValue(color);
    DWORD const g = GetGValue(color);
    DWORD const b = GetBValue(color);

    // An example string with max length would be "\x1b[38;2;128;128;128m", which has length = 19.
    char buf[19];
    char* end = fmt::format_to(std::begin(buf), FMT_COMPILE("\x1b[{};2;{};{};{}m"), fIsForeground ? 38 : 48, r, g, b);

    return _Write({ buf, static_cast<std::string_view::size_type>(end - buf) });
}

// Method Description:

Review comment (bug):
I'm strongly concerned with the memory safety of this code.
First of all you forgot to include the 0 byte in your buffer size calculation. I'm not trying to diss you here, rather this simply shows how such things are best abstracted away behind safer container classes. Even if the `buf` was 12 bytes large, it would fail the moment anyone touches the code and the reviewer forgets to re-check the correctness of the buffer size.
Additionally `Xterm256ToWindowsIndex` can theoretically return values as large as 2^16-1, resulting in a buffer size of 14!

MSVC's small string optimization (SSO) is unfortunately among the worst of all C++ stdlib implementations. But regardless its 15 byte buffer is plenty enough for both `_SetGraphicsRendition256Color` and `_SetGraphicsRendition16Color`, and allows you to use the regular `fmt::format` function without incurring any allocations.

👉 I'm convinced you should use `fmt::format()` here.

Code after:
    return _WriteFormatted(FMT_COMPILE("\x1b[{}m"), vtIndex);
}

// Method Description:
// - Formats and writes a sequence to change the current text attributes to an
//      indexed color from the 256-color table.
// Arguments:
// - wAttr: Windows color table index to emit as a VT sequence
// - fIsForeground: true if we should emit the foreground sequence, false for background
// Return Value:
// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
[[nodiscard]] HRESULT VtEngine::_SetGraphicsRendition256Color(const WORD index,
                                                              const bool fIsForeground) noexcept
{
    return _WriteFormatted(FMT_COMPILE("\x1b[{}8;5;{}m"), fIsForeground ? '3' : '4', ::Xterm256ToWindowsIndex(index));
}

// Method Description:
// - Formats and writes a sequence to change the current text attributes to an
//      RGB color.
// Arguments:
// - color: The color to emit a VT sequence for
// - fIsForeground: true if we should emit the foreground sequence, false for background
// Return Value:
// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
[[nodiscard]] HRESULT VtEngine::_SetGraphicsRenditionRGBColor(const COLORREF color,
                                                              const bool fIsForeground) noexcept
{
    const uint8_t r = GetRValue(color);
    const uint8_t g = GetGValue(color);
    const uint8_t b = GetBValue(color);
    return _WriteFormatted(FMT_COMPILE("\x1b[{}8;2;{};{};{}m"), fIsForeground ? '3' : '4', r, g, b);
}

// Method Description:
// - Formats and writes a sequence to change the current text attributes to the
//      default foreground or background. Does not affect the boldness of text.
// Arguments:
// - fIsForeground: true if we should emit the foreground sequence, false for background
// Return Value:
// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
[[nodiscard]] HRESULT VtEngine::_SetGraphicsRenditionDefaultColor(const bool fIsForeground) noexcept
{
    return _Write(fIsForeground ? ("\x1b[39m") : ("\x1b[49m"));
}

// Method Description:
// - Formats and writes a sequence to change the terminal's window size.
// Arguments:
// - sWidth: number of columns the terminal should display
// - sHeight: number of rows the terminal should display