ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/terminal
File: src/buffer/out/TextAttribute.cpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"
#include "TextAttribute.hpp"
#include "../../inc/conattrs.hpp"

// Keeping TextColor compact helps us keeping TextAttribute compact,
// which in turn ensures that our buffer memory usage is low.
static_assert(sizeof(TextAttribute) == 14);
static_assert(alignof(TextAttribute) == 2);
// Ensure that we can memcpy() and memmove() the struct for performance.
static_assert(std::is_trivially_copyable_v<TextAttribute>);
// Assert that the use of memcmp() for comparisons is safe.
static_assert(std::has_unique_object_representations_v<TextAttribute>);

namespace
{
    constexpr std::array<TextColor, 16> s_initLegacyColorMap(const BYTE defaultIndex)
    {
        std::array<TextColor, 16> legacyColorMap;
        for (auto i = 0u; i < legacyColorMap.size(); i++)
        {
            const auto legacyIndex = TextColor::TransposeLegacyIndex(i);
            gsl::at(legacyColorMap, i) = i == defaultIndex ? TextColor{} : TextColor{ legacyIndex, true };
        }
        return legacyColorMap;
    }

    BYTE s_legacyDefaultForeground = 7;
    BYTE s_legacyDefaultBackground = 0;
    BYTE s_ansiDefaultForeground = 7;
    BYTE s_ansiDefaultBackground = 0;
}


Review comment (suggestion):
A memory savings of 2 megabytes, assuming that every column in every row in the default conhost buffer had 120 different attributes. 😁 

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"
#include "TextAttribute.hpp"
#include "../../inc/conattrs.hpp"

// Keeping TextColor compact helps us keeping TextAttribute compact,
// which in turn ensures that our buffer memory usage is low.
static_assert(sizeof(TextAttribute) == 12);
static_assert(alignof(TextAttribute) == 2);
// Ensure that we can memcpy() and memmove() the struct for performance.
static_assert(std::is_trivially_copyable_v<TextAttribute>);
// Assert that the use of memcmp() for comparisons is safe.
static_assert(std::has_unique_object_representations_v<TextAttribute>);

namespace
{
    constexpr std::array<TextColor, 16> s_initLegacyColorMap(const BYTE defaultIndex)
    {
        std::array<TextColor, 16> legacyColorMap;
        for (auto i = 0u; i < legacyColorMap.size(); i++)
        {
            const auto legacyIndex = TextColor::TransposeLegacyIndex(i);
            gsl::at(legacyColorMap, i) = i == defaultIndex ? TextColor{} : TextColor{ legacyIndex, true };
        }
        return legacyColorMap;
    }

    BYTE s_legacyDefaultForeground = 7;
    BYTE s_legacyDefaultBackground = 0;
    BYTE s_ansiDefaultForeground = 7;
    BYTE s_ansiDefaultBackground = 0;
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsModel/SshHostGenerator.h (C/C++)

Code before:
- Jon Thysell - September 2022

--*/

#pragma once

#include "IDynamicProfileGenerator.h"

namespace winrt::Microsoft::Terminal::Settings::Model
{
    class SshHostGenerator final : public IDynamicProfileGenerator
    {
    public:
        std::wstring_view GetNamespace() const noexcept override;
        void GenerateProfiles(std::vector<winrt::com_ptr<implementation::Profile>>& profiles) const override;

    private:
        static const std::wregex ConfigKeyValueRegex;

        static std::wstring GetProfileName(const std::wstring_view& hostName) noexcept;
        static std::wstring GetProfileIconPath() noexcept;
        static std::wstring GetProfileCommandLine(const std::wstring_view& sshExePath, const std::wstring_view& hostName) noexcept;

        static bool TryFindSshExePath(std::wstring& sshExePath) noexcept;
        static bool TryParseConfigKeyValue(const std::wstring_view& line, std::wstring& key, std::wstring& value) noexcept;
        static void GetHostNamesFromConfigFile(const std::wstring configPath, std::vector<std::wstring>& hostNames) noexcept;
    };
};


Review comment (suggestion):
```suggestion
        static std::wstring _GetProfileName(const std::wstring_view& hostName) noexcept;
        static std::wstring _GetProfileIconPath() noexcept;
        static std::wstring _GetProfileCommandLine(const std::wstring_view& sshExePath, const std::wstring_view& hostName) noexcept;

        static bool _TryFindSshExePath(std::wstring& sshExePath) noexcept;
        static bool _TryParseConfigKeyValue(const std::wstring_view& line, std::wstring& key, std::wstring& value) noexcept;
        static void _GetHostNamesFromConfigFile(const std::wstring configPath, std::vector<std::wstring>& hostNames) noexcept;
```
nit: we generally have private func start with `_`

Code after:
- Jon Thysell - September 2022

--*/

#pragma once

#include "IDynamicProfileGenerator.h"

namespace winrt::Microsoft::Terminal::Settings::Model
{
    class SshHostGenerator final : public IDynamicProfileGenerator
    {
    public:
        std::wstring_view GetNamespace() const noexcept override;
        void GenerateProfiles(std::vector<winrt::com_ptr<implementation::Profile>>& profiles) const override;

    private:
        static const std::wregex _configKeyValueRegex;

        static std::wstring_view _getProfileName(const std::wstring_view& hostName) noexcept;
        static std::wstring_view _getProfileIconPath() noexcept;
        static std::wstring_view _getProfileCommandLine(const std::wstring_view& sshExePath, const std::wstring_view& hostName) noexcept;

        static bool _tryFindSshExePath(std::wstring& sshExePath) noexcept;
        static bool _tryParseConfigKeyValue(const std::wstring_view& line, std::wstring& key, std::wstring& value) noexcept;
        static void _getHostNamesFromConfigFile(const std::wstring_view& configPath, std::vector<std::wstring>& hostNames) noexcept;
    };
};
Repository: microsoft/terminal
File: src/cascadia/PublicTerminalCore/HwndTerminal.cpp (C++)

Code before:
        ScreenToClient(_hwnd.get(), cursorPosition.as_win32_point());
    }

    const TerminalInput::MouseButtonState state{
        WI_IsFlagSet(GetKeyState(VK_LBUTTON), KeyPressed),
        WI_IsFlagSet(GetKeyState(VK_MBUTTON), KeyPressed),
        WI_IsFlagSet(GetKeyState(VK_RBUTTON), KeyPressed)
    };

    return _terminal->SendMouseEvent(cursorPosition / fontSize, uMsg, getControlKeyState(), wheelDelta, state);
}
catch (...)
{
    LOG_CAUGHT_EXCEPTION();
    return false;
}

void HwndTerminal::_SendKeyEvent(WORD vkey, WORD scanCode, WORD flags, bool keyDown) noexcept
try
{
    auto modifiers = getControlKeyState();
    if (WI_IsFlagSet(flags, ENHANCED_KEY))
    {
        modifiers |= ControlKeyStates::EnhancedKey;
    }
    if (vkey && keyDown && _uiaProviderInitialized)
    {
        _uiaProvider->RecordKeyEvent(vkey);
    }
    _terminal->SendKeyEvent(vkey, scanCode, modifiers, keyDown);
}
CATCH_LOG();

void HwndTerminal::_SendCharEvent(wchar_t ch, WORD scanCode, WORD flags) noexcept
try
{
    if (_terminal->IsSelectionActive())
    {
        _ClearSelection();
        if (ch == UNICODE_ESC)
        {
            // ESC should clear any selection before it triggers input.
            // Other characters pass through.
            return;
        }
    }

    if (ch == UNICODE_TAB)
    {
        // TAB was handled as a keydown event (cf. Terminal::SendKeyEvent)
        return;

Review comment (suggestion):
Considering doing a quick replacement of `_uiaProviderInitialized` --> `_uiaProvider` null check. Should accomplish the same thing.

Code after:
    LOG_CAUGHT_EXCEPTION();
    return false;
}

void HwndTerminal::_SendKeyEvent(WORD vkey, WORD scanCode, WORD flags, bool keyDown) noexcept
try
{
    auto modifiers = getControlKeyState();
    if (WI_IsFlagSet(flags, ENHANCED_KEY))
    {
        modifiers |= ControlKeyStates::EnhancedKey;
    }
    if (vkey && keyDown && _uiaProvider)
    {
        _uiaProvider->RecordKeyEvent(vkey);
    }
    _terminal->SendKeyEvent(vkey, scanCode, modifiers, keyDown);
}
CATCH_LOG();

void HwndTerminal::_SendCharEvent(wchar_t ch, WORD scanCode, WORD flags) noexcept
try
{
    if (_terminal->IsSelectionActive())
    {
        _ClearSelection();
        if (ch == UNICODE_ESC)
        {
            // ESC should clear any selection before it triggers input.
            // Other characters pass through.
            return;
        }
    }

    if (ch == UNICODE_TAB)
    {
        // TAB was handled as a keydown event (cf. Terminal::SendKeyEvent)
        return;
    }

    auto modifiers = getControlKeyState();
    if (WI_IsFlagSet(flags, ENHANCED_KEY))
    {
        modifiers |= ControlKeyStates::EnhancedKey;
    }
    _terminal->SendCharEvent(ch, scanCode, modifiers);
}
CATCH_LOG();

void _stdcall TerminalSendKeyEvent(void* terminal, WORD vkey, WORD scanCode, WORD flags, bool keyDown)
{
Repository: microsoft/terminal
File: src/cascadia/PublicTerminalCore/HwndTerminalAutomationPeer.cpp (C++)

Code before:
        {
            // the key event's character (i.e. the "A" key) matches
            // the output character (i.e. "a" or "A" text).
            // We can assume that the output character resulted from
            // the pressed key, so we can ignore it.
            sanitized = sanitized.substr(1);
            _keyEvents.pop_front();
        }
        else
        {
            // The output doesn't match,
            // so clear the input stack and
            // move on to fire the event.
            _keyEvents.clear();
            break;
        }
    }

    // Suppress event if the remaining text is not readable
    if (!IsReadable(sanitized))
    {
        return;
    }

    const auto sanitizedBstr = SysAllocString(sanitized.c_str());
    static auto activityId = SysAllocString(L"TerminalTextOutput");
    LOG_IF_FAILED(UiaRaiseNotificationEvent(this, NotificationKind_ActionCompleted, NotificationProcessing_All, sanitizedBstr, activityId));
}


Review comment (suggestion):
we might be leaking these -- can you confirm whether UiaRaise takes ownership? I bet it doesn't.

Should use `wil::unique_bstr`

Code after:
        {
            // the key event's character (i.e. the "A" key) matches
            // the output character (i.e. "a" or "A" text).
            // We can assume that the output character resulted from
            // the pressed key, so we can ignore it.
            sanitized = sanitized.substr(1);
            _keyEvents.pop_front();
        }
        else
        {
            // The output doesn't match,
            // so clear the input stack and
            // move on to fire the event.
            _keyEvents.clear();
            break;
        }
    }

    // Suppress event if the remaining text is not readable
    if (!IsReadable(sanitized))
    {
        return;
    }

    const auto sanitizedBstr = wil::make_bstr_nothrow(sanitized.c_str());
    static auto activityId = wil::make_bstr_nothrow(L"TerminalTextOutput");
    LOG_IF_FAILED(UiaRaiseNotificationEvent(this, NotificationKind_ActionCompleted, NotificationProcessing_All, sanitizedBstr.get(), activityId.get()));
}
Repository: microsoft/terminal
File: src/cascadia/PublicTerminalCore/HwndTerminalAutomationPeer.cpp (C++)

Code before:
        {
            // the key event's character (i.e. the "A" key) matches
            // the output character (i.e. "a" or "A" text).
            // We can assume that the output character resulted from
            // the pressed key, so we can ignore it.
            sanitized = sanitized.substr(1);
            _keyEvents.pop_front();
        }
        else
        {
            // The output doesn't match,
            // so clear the input stack and
            // move on to fire the event.
            _keyEvents.clear();
            break;
        }
    }

    // Suppress event if the remaining text is not readable
    if (!IsReadable(sanitized))
    {
        return;
    }

    const auto sanitizedBstr = wil::unique_bstr(sanitized.c_str());
    static auto activityId = wil::unique_bstr(L"TerminalTextOutput");
    LOG_IF_FAILED(UiaRaiseNotificationEvent(this, NotificationKind_ActionCompleted, NotificationProcessing_All, sanitizedBstr.get(), activityId.get()));
}


Review comment (suggestion):
ah, this is a bit of a weirdness.

I don't know if this is a legal way to create a unique_bstr.

Try using `wil::make_bstr_nothrow()` instead!

Code after:
        {
            // the key event's character (i.e. the "A" key) matches
            // the output character (i.e. "a" or "A" text).
            // We can assume that the output character resulted from
            // the pressed key, so we can ignore it.
            sanitized = sanitized.substr(1);
            _keyEvents.pop_front();
        }
        else
        {
            // The output doesn't match,
            // so clear the input stack and
            // move on to fire the event.
            _keyEvents.clear();
            break;
        }
    }

    // Suppress event if the remaining text is not readable
    if (!IsReadable(sanitized))
    {
        return;
    }

    const auto sanitizedBstr = wil::make_bstr_nothrow(sanitized.c_str());
    static auto activityId = wil::make_bstr_nothrow(L"TerminalTextOutput");
    LOG_IF_FAILED(UiaRaiseNotificationEvent(this, NotificationKind_ActionCompleted, NotificationProcessing_All, sanitizedBstr.get(), activityId.get()));
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsModel/IconPathConverter.cpp (C++)

Code before:
        bitmapSource.SetBitmapAsync(swBitmap);
        return bitmapSource;
    }

    MUX::Controls::IconSource IconPathConverter::IconSourceMUX(const winrt::hstring& iconPath)
    {
        std::wstring_view iconPathWithoutIndex;
        auto indexOpt = _getIconIndex(iconPath, iconPathWithoutIndex);
        if (!indexOpt.has_value())
        {
            return _IconSourceMUX(iconPath);
        }
        auto index = indexOpt.value();

        auto bitmapSource = _getImageIconSourceForBinaryNotAsync(iconPathWithoutIndex, index);

        MUX::Controls::ImageIconSource imageIconSource{};
        imageIconSource.ImageSource(bitmapSource);

        return imageIconSource;
    }

    Windows::UI::Xaml::Controls::IconElement IconPathConverter::IconWUX(const winrt::hstring& iconPath)
    {
        std::wstring_view iconPathWithoutIndex;
        auto indexOpt = _getIconIndex(iconPath, iconPathWithoutIndex);
        if (!indexOpt.has_value())
        {
            auto source = _IconSourceWUX(iconPath);
            Controls::IconSourceElement icon;
            icon.IconSource(source);
            icon.Width(32);
            icon.Height(32);
            return icon;
        }
        auto index = indexOpt.value();

        auto bitmapSource = _getImageIconSourceForBinaryNotAsync(iconPathWithoutIndex, index);

        winrt::Microsoft::UI::Xaml::Controls::ImageIcon icon{};
        icon.Source(bitmapSource);
        icon.Width(32);
        icon.Height(32);
        return icon;
    }
}


Review comment (suggestion):
```suggestion
        const auto indexOpt = _getIconIndex(iconPath, iconPathWithoutIndex);
```

Code after:
            return _IconSourceMUX(iconPath);
        }

        const auto bitmapSource = _getImageIconSourceForBinary(iconPathWithoutIndex, indexOpt.value());

        MUX::Controls::ImageIconSource imageIconSource{};
        imageIconSource.ImageSource(bitmapSource);

        return imageIconSource;
    }

    Windows::UI::Xaml::Controls::IconElement IconPathConverter::IconWUX(const winrt::hstring& iconPath)
    {
        std::wstring_view iconPathWithoutIndex;
        const auto indexOpt = _getIconIndex(iconPath, iconPathWithoutIndex);
        if (!indexOpt.has_value())
        {
            auto source = _IconSourceWUX(iconPath);
            Controls::IconSourceElement icon;
            icon.IconSource(source);
            return icon;
        }

        const auto bitmapSource = _getImageIconSourceForBinary(iconPathWithoutIndex, indexOpt.value());

        winrt::Microsoft::UI::Xaml::Controls::ImageIcon icon{};
        icon.Source(bitmapSource);
        icon.Width(32);
        icon.Height(32);
        return icon;
    }
}
Repository: microsoft/terminal
File: src/interactivity/base/RemoteConsoleControl.cpp (C++)

Code before:
}

[[nodiscard]] NTSTATUS RemoteConsoleControl::SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground)
{
    // GH#13211 - Apparently this API doesn't need to be forwarded to conhost at
    // all. Instead, just perform the ConsoleControl operation here, in proc.
    // This lets us avoid all sorts of strange handle duplicating weirdness.
    return _control.SetForeground(hProcess, fForeground);
}

[[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags)
{
    HostSignalEndTaskData data{};
    data.sizeInBytes = sizeof(data);
    data.processId = dwProcessId;
    data.eventType = dwEventType;
    data.ctrlFlags = ulCtrlFlags;

    return _SendTypedPacket(_pipe.get(), HostSignals::EndTask, data);
}

[[nodiscard]] NTSTATUS RemoteConsoleControl::SetWindowOwner(HWND hwnd, DWORD processId, DWORD threadId)
{
    return _control.SetWindowOwner(hwnd, processId, threadId);
}

#pragma endregion


Review comment (suggestion):
```suggestion
    // This call doesn't need to get forwarded to the root conhost. Just handle
    // it in-proc, to set the owner of OpenConsole
    return _control.SetWindowOwner(hwnd, processId, threadId);
```

Code after:
}

[[nodiscard]] NTSTATUS RemoteConsoleControl::SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground)
{
    // GH#13211 - Apparently this API doesn't need to be forwarded to conhost at
    // all. Instead, just perform the ConsoleControl operation here, in proc.
    // This lets us avoid all sorts of strange handle duplicating weirdness.
    return _control.SetForeground(hProcess, fForeground);
}

[[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags)
{
    HostSignalEndTaskData data{};
    data.sizeInBytes = sizeof(data);
    data.processId = dwProcessId;
    data.eventType = dwEventType;
    data.ctrlFlags = ulCtrlFlags;

    return _SendTypedPacket(_pipe.get(), HostSignals::EndTask, data);
}

[[nodiscard]] NTSTATUS RemoteConsoleControl::SetWindowOwner(HWND hwnd, DWORD processId, DWORD threadId)
{
    // This call doesn't need to get forwarded to the root conhost. Just handle
    // it in-proc, to set the owner of OpenConsole
    return _control.SetWindowOwner(hwnd, processId, threadId);
}

#pragma endregion
Repository: microsoft/terminal
File: src/terminal/adapter/DispatchTypes.hpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

namespace Microsoft::Console::VirtualTerminal
{
    using VTInt = int32_t;
    using MidiDuration = std::chrono::milliseconds;

    class VTID
    {
    public:
        template<size_t Length>
        constexpr VTID(const char (&s)[Length]) :
            _value{ _FromString(s) }
        {
        }

        constexpr VTID(const uint64_t value) :
            _value{ value }
        {
        }

        constexpr operator uint64_t() const
        {
            return _value;
        }

        constexpr char operator[](const size_t offset) const
        {
            return SubSequence(offset)._value & 0xFF;
        }


Review comment (suggestion):
Nit: I think the duration parameters would be better left as they were with the original microsecond precision, and then use an explicit `duration_cast` to get the milliseconds when you need them. Maybe one day we might even be able to use the exact duration value for the wait. As it is, we're left with a `count()` of an unknown scale, which is kind of meaningless. 

Also having to use `::Microsoft::Console::VirtualTerminal::MidiDuration` all over the place is bit painful.

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

namespace Microsoft::Console::VirtualTerminal
{
    using VTInt = int32_t;

    class VTID
    {
    public:
        template<size_t Length>
        constexpr VTID(const char (&s)[Length]) :
            _value{ _FromString(s) }
        {
        }

        constexpr VTID(const uint64_t value) :
            _value{ value }
        {
        }

        constexpr operator uint64_t() const
        {
            return _value;
        }

        constexpr char operator[](const size_t offset) const
        {
            return SubSequence(offset)._value & 0xFF;
        }

        constexpr VTID SubSequence(const size_t offset) const
Repository: microsoft/terminal
File: src/host/VtInputThread.cpp (C++)

Code before:
// Return Value:
// - The return value of the underlying instance's _InputThread
DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter)
{
    const auto pInstance = reinterpret_cast<VtInputThread*>(lpParameter);
    pInstance->_InputThread();
}

// Method Description:
// - Do a single ReadFile from our pipe, and try and handle it. If handling
//      failed, throw or log, depending on what the caller wants.
// Arguments:
// - throwOnFail: If true, throw an exception if there was an error processing
//      the input received. Otherwise, log the error.
// Return Value:
// - <none>
void VtInputThread::DoReadInput(const bool throwOnFail)
{
    char buffer[256];
    DWORD dwRead = 0;
    auto fSuccess = !!ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr);

    if (!fSuccess)
    {
        _exitRequested = true;
        return;
    }

    auto hr = _HandleRunInput({ buffer, gsl::narrow_cast<size_t>(dwRead) });
    if (FAILED(hr))
    {
        if (throwOnFail)
        {
            _exitRequested = true;
        }
        else
        {
            LOG_IF_FAILED(hr);
        }
    }
}

void VtInputThread::SetLookingForDSR(const bool looking) noexcept
{
    if (_pfnSetLookingForDSR)
    {
        _pfnSetLookingForDSR(looking);
    }
}

// Method Description:

Review comment (suggestion):
This comment is a copy/paste from `PtySignalInputThread`, but it doesn't actually apply to this code. Also there's only 4 uses of `_exitRequested` in total which makes this code easy to follow anyways IMO.

Code after:
// Return Value:
// - The return value of the underlying instance's _InputThread
DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter)
{
    const auto pInstance = reinterpret_cast<VtInputThread*>(lpParameter);
    pInstance->_InputThread();
    return S_OK;
}

// Method Description:
// - Do a single ReadFile from our pipe, and try and handle it. If handling
//      failed, throw or log, depending on what the caller wants.
// Arguments:
// - throwOnFail: If true, throw an exception if there was an error processing
//      the input received. Otherwise, log the error.
// Return Value:
// - <none>
void VtInputThread::DoReadInput(const bool throwOnFail)
{
    char buffer[256];
    DWORD dwRead = 0;
    auto fSuccess = !!ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr);

    if (!fSuccess)
    {
        _exitRequested = true;
        return;
    }

    auto hr = _HandleRunInput({ buffer, gsl::narrow_cast<size_t>(dwRead) });
    if (FAILED(hr))
    {
        if (throwOnFail)
        {
            _exitRequested = true;
        }
        else
        {
            LOG_IF_FAILED(hr);
        }
    }
}

void VtInputThread::SetLookingForDSR(const bool looking) noexcept
{
    if (_pfnSetLookingForDSR)
    {
        _pfnSetLookingForDSR(looking);
    }
}