ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/terminal
File: src/host/PtySignalInputThread.hpp (C++)

Code before:
        enum class PtySignal : unsigned short
        {
            ShowHideWindow = 1,
            ClearBuffer = 2,
            SetParent = 3,
            ResizeWindow = 8
        };

        struct ResizeWindowData
        {
            unsigned short sx;
            unsigned short sy;
        };

        struct ShowHideData
        {
            unsigned short show; // used as a bool, but passed as a ushort
        };

        struct SetParentData
        {
            uint64_t handle;
        };

        [[nodiscard]] HRESULT _InputThread() noexcept;
        bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer);
        void _DoResizeWindow(const ResizeWindowData& data);
        void _DoSetWindowParent(const SetParentData& data);
        void _DoClearBuffer();
        void _DoShowHide(const bool show);
        void _Shutdown();

        wil::unique_hfile _hFile;
        wil::unique_handle _hThread;
        DWORD _dwThreadId;
        bool _consoleConnected;
        std::optional<ResizeWindowData> _earlyResize;
        std::optional<ShowHideData> _initialShowHide;
        ConhostInternalGetSet _api;

    public:
        std::optional<SetParentData> _earlyReparent;
    };
}


Review comment (suggestion):
```suggestion
        [[nodiscard]] bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer);
```
Looks like everybody explicitly handles `_GetData()`'s return val (which imo is much better). You could add the `nodiscard` attribute if you want.

Code after:
        enum class PtySignal : unsigned short
        {
            ShowHideWindow = 1,
            ClearBuffer = 2,
            SetParent = 3,
            ResizeWindow = 8
        };

        struct ResizeWindowData
        {
            unsigned short sx;
            unsigned short sy;
        };

        struct ShowHideData
        {
            unsigned short show; // used as a bool, but passed as a ushort
        };

        struct SetParentData
        {
            uint64_t handle;
        };

        [[nodiscard]] HRESULT _InputThread() noexcept;
        [[nodiscard]] bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer);
        void _DoResizeWindow(const ResizeWindowData& data);
        void _DoSetWindowParent(const SetParentData& data);
        void _DoClearBuffer();
        void _DoShowHide(const bool show);
        void _Shutdown();

        wil::unique_hfile _hFile;
        wil::unique_handle _hThread;
        DWORD _dwThreadId;
        bool _consoleConnected;
        std::optional<ResizeWindowData> _earlyResize;
        std::optional<ShowHideData> _initialShowHide;
        ConhostInternalGetSet _api;

    public:
        std::optional<SetParentData> _earlyReparent;
    };
}
Repository: microsoft/terminal
File: src/server/ApiDispatchersInternal.cpp (C++)

Code before:

    // Make sure the process group id is valid.
    if (a->ProcessGroupId != 0)
    {
        ConsoleProcessHandle* ProcessHandle;
        ProcessHandle = gci.ProcessHandleList.FindProcessByGroupId(a->ProcessGroupId);
        if (ProcessHandle == nullptr)
        {
            auto ProcessId = a->ProcessGroupId;

            // We didn't find a process with that group ID.
            // Let's see if the process with that ID exists and has a parent that is a member of this console.
            RETURN_IF_NTSTATUS_FAILED((NtPrivApi::s_GetProcessParentId(&ProcessId)));
            ProcessHandle = gci.ProcessHandleList.FindProcessInList(ProcessId);
            RETURN_HR_IF_NULL(E_INVALIDARG, ProcessHandle);

            const auto hr = gci.ProcessHandleList.AllocProcessData(a->ProcessGroupId, 0, a->ProcessGroupId, nullptr);
            // AllocProcessData returns E_FAIL if the given process ID already exists.
            // In that case we don't really need to return an error and can just carry on.
            if (FAILED(hr) && hr != E_FAIL)
            {
                RETURN_HR(hr);
            }
        }
    }

    gci.LimitingProcessId = a->ProcessGroupId;
    HandleCtrlEvent(a->CtrlEvent);

    return S_OK;
}


Review comment (bug):
Now that `AllocProcessData` consistently returns `E_FAIL` if the process ID already exists, we need this bit of extra code here. TBH I don't quite understand why this adds a "ProcessGroupId" as a process ID to the process list here...

Code after:

    // Make sure the process group id is valid.
    if (a->ProcessGroupId != 0)
    {
        ConsoleProcessHandle* ProcessHandle;
        ProcessHandle = gci.ProcessHandleList.FindProcessByGroupId(a->ProcessGroupId);
        if (ProcessHandle == nullptr)
        {
            auto ProcessId = a->ProcessGroupId;

            // We didn't find a process with that group ID.
            // Let's see if the process with that ID exists and has a parent that is a member of this console.
            RETURN_IF_NTSTATUS_FAILED((NtPrivApi::s_GetProcessParentId(&ProcessId)));
            ProcessHandle = gci.ProcessHandleList.FindProcessInList(ProcessId);
            RETURN_HR_IF_NULL(E_INVALIDARG, ProcessHandle);
            RETURN_IF_FAILED(gci.ProcessHandleList.AllocProcessData(a->ProcessGroupId,
                                                                    0,
                                                                    a->ProcessGroupId,
                                                                    nullptr));
        }
    }

    gci.LimitingProcessId = a->ProcessGroupId;
    HandleCtrlEvent(a->CtrlEvent);

    return S_OK;
}
Repository: microsoft/terminal
File: src/server/ApiDispatchersInternal.cpp (C++)

Code before:
    Telemetry::Instance().LogApiCall(Telemetry::ApiCall::GenerateConsoleCtrlEvent);

    LockConsole();
    auto Unlock = wil::scope_exit([&] { UnlockConsole(); });

    // Make sure the process group id is valid.
    if (a->ProcessGroupId != 0)
    {
        ConsoleProcessHandle* ProcessHandle;
        ProcessHandle = gci.ProcessHandleList.FindProcessByGroupId(a->ProcessGroupId);
        if (ProcessHandle == nullptr)
        {
            auto ProcessId = a->ProcessGroupId;

            // We didn't find a process with that group ID.
            // Let's see if the process with that ID exists and has a parent that is a member of this console.
            RETURN_IF_NTSTATUS_FAILED((NtPrivApi::s_GetProcessParentId(&ProcessId)));
            ProcessHandle = gci.ProcessHandleList.FindProcessInList(ProcessId);
            RETURN_HR_IF_NULL(E_INVALIDARG, ProcessHandle);

            const auto hr = gci.ProcessHandleList.AllocProcessData(a->ProcessGroupId,
                                                                   0,
                                                                   a->ProcessGroupId,
                                                                   nullptr);
            // AllocProcessData returns E_FAIL if the given process ID already exists.
            // The process ID already existing isn't really an error though so we ignore it.
            if (FAILED(hr) && hr != E_FAIL)
            {
                RETURN_HR(hr);
            }
        }
    }

    gci.LimitingProcessId = a->ProcessGroupId;
    HandleCtrlEvent(a->CtrlEvent);

    return S_OK;
}


Review comment (suggestion):
you could use `S_FALSE` for a non-error case if want

Code after:
    Telemetry::Instance().LogApiCall(Telemetry::ApiCall::GenerateConsoleCtrlEvent);

    LockConsole();
    auto Unlock = wil::scope_exit([&] { UnlockConsole(); });

    // Make sure the process group id is valid.
    if (a->ProcessGroupId != 0)
    {
        ConsoleProcessHandle* ProcessHandle;
        ProcessHandle = gci.ProcessHandleList.FindProcessByGroupId(a->ProcessGroupId);
        if (ProcessHandle == nullptr)
        {
            auto ProcessId = a->ProcessGroupId;

            // We didn't find a process with that group ID.
            // Let's see if the process with that ID exists and has a parent that is a member of this console.
            RETURN_IF_NTSTATUS_FAILED((NtPrivApi::s_GetProcessParentId(&ProcessId)));
            ProcessHandle = gci.ProcessHandleList.FindProcessInList(ProcessId);
            RETURN_HR_IF_NULL(E_INVALIDARG, ProcessHandle);
            RETURN_IF_FAILED(gci.ProcessHandleList.AllocProcessData(a->ProcessGroupId,
                                                                    0,
                                                                    a->ProcessGroupId,
                                                                    nullptr));
        }
    }

    gci.LimitingProcessId = a->ProcessGroupId;
    HandleCtrlEvent(a->CtrlEvent);

    return S_OK;
}
Repository: microsoft/terminal
File: src/buffer/out/search.h (C/C++)

Code before:

#pragma once

#include "TextAttribute.hpp"
#include "textBuffer.hpp"
#include "../renderer/inc/IRenderData.hpp"

// This used to be in find.h.
#define SEARCH_STRING_LENGTH (80)

class Search final
{
public:
    enum class Direction
    {
        Forward,
        Backward
    };

    enum class Sensitivity
    {
        CaseInsensitive,
        CaseSensitive
    };

    Search(Microsoft::Console::Render::IRenderData& uiaData,
           const std::wstring_view str,
           const Direction dir,
           const Sensitivity sensitivity);

    Search(Microsoft::Console::Render::IRenderData& uiaData,
           const std::wstring_view str,
           const Direction dir,
           const Sensitivity sensitivity,
           const til::point anchor);

    bool FindNext();
    void Select() const;
    void Color(const TextAttribute attr) const;

    std::pair<til::point, til::point> GetFoundLocation() const noexcept;

private:
    wchar_t _ApplySensitivity(const wchar_t wch) const noexcept;
    bool _FindNeedleInHaystackAt(const til::point pos, til::point& start, til::point& end) const;
    bool _CompareChars(const std::wstring_view one, const std::wstring_view two) const noexcept;
    void _UpdateNextPosition();

    void _IncrementCoord(til::point& coord) const noexcept;
    void _DecrementCoord(til::point& coord) const noexcept;


Review comment (suggestion):
```suggestion
    Search(Microsoft::Console::Render::IRenderData& renderData,
```

Code after:

#pragma once

#include "TextAttribute.hpp"
#include "textBuffer.hpp"
#include "../renderer/inc/IRenderData.hpp"

// This used to be in find.h.
#define SEARCH_STRING_LENGTH (80)

class Search final
{
public:
    enum class Direction
    {
        Forward,
        Backward
    };

    enum class Sensitivity
    {
        CaseInsensitive,
        CaseSensitive
    };

    Search(Microsoft::Console::Render::IRenderData& renderData,
           const std::wstring_view str,
           const Direction dir,
           const Sensitivity sensitivity);

    Search(Microsoft::Console::Render::IRenderData& renderData,
           const std::wstring_view str,
           const Direction dir,
           const Sensitivity sensitivity,
           const til::point anchor);

    bool FindNext();
    void Select() const;
    void Color(const TextAttribute attr) const;

    std::pair<til::point, til::point> GetFoundLocation() const noexcept;

private:
    wchar_t _ApplySensitivity(const wchar_t wch) const noexcept;
    bool _FindNeedleInHaystackAt(const til::point pos, til::point& start, til::point& end) const;
    bool _CompareChars(const std::wstring_view one, const std::wstring_view two) const noexcept;
    void _UpdateNextPosition();

    void _IncrementCoord(til::point& coord) const noexcept;
    void _DecrementCoord(til::point& coord) const noexcept;
Repository: microsoft/terminal
File: src/buffer/out/search.h (C/C++)

Code before:

    std::pair<til::point, til::point> GetFoundLocation() const noexcept;

private:
    wchar_t _ApplySensitivity(const wchar_t wch) const noexcept;
    bool _FindNeedleInHaystackAt(const til::point pos, til::point& start, til::point& end) const;
    bool _CompareChars(const std::wstring_view one, const std::wstring_view two) const noexcept;
    void _UpdateNextPosition();

    void _IncrementCoord(til::point& coord) const noexcept;
    void _DecrementCoord(til::point& coord) const noexcept;

    static til::point s_GetInitialAnchor(const Microsoft::Console::Render::IRenderData& uiaData, const Direction dir);

    static std::vector<std::vector<wchar_t>> s_CreateNeedleFromString(const std::wstring_view wstr);

    bool _reachedEnd = false;
    til::point _coordNext;
    til::point _coordSelStart;
    til::point _coordSelEnd;

    const til::point _coordAnchor;
    const std::vector<std::vector<wchar_t>> _needle;
    const Direction _direction;
    const Sensitivity _sensitivity;
    Microsoft::Console::Render::IRenderData& _uiaData;

#ifdef UNIT_TESTING
    friend class SearchTests;
#endif
};


Review comment (suggestion):
```suggestion
    Microsoft::Console::Render::IRenderData& _renderData;
```

Code after:

    std::pair<til::point, til::point> GetFoundLocation() const noexcept;

private:
    wchar_t _ApplySensitivity(const wchar_t wch) const noexcept;
    bool _FindNeedleInHaystackAt(const til::point pos, til::point& start, til::point& end) const;
    bool _CompareChars(const std::wstring_view one, const std::wstring_view two) const noexcept;
    void _UpdateNextPosition();

    void _IncrementCoord(til::point& coord) const noexcept;
    void _DecrementCoord(til::point& coord) const noexcept;

    static til::point s_GetInitialAnchor(const Microsoft::Console::Render::IRenderData& renderData, const Direction dir);

    static std::vector<std::wstring> s_CreateNeedleFromString(const std::wstring_view wstr);

    bool _reachedEnd = false;
    til::point _coordNext;
    til::point _coordSelStart;
    til::point _coordSelEnd;

    const til::point _coordAnchor;
    const std::vector<std::wstring> _needle;
    const Direction _direction;
    const Sensitivity _sensitivity;
    Microsoft::Console::Render::IRenderData& _renderData;

#ifdef UNIT_TESTING
    friend class SearchTests;
#endif
};
Repository: microsoft/terminal
File: src/interactivity/win32/windowUiaProvider.cpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"

#include "screenInfoUiaProvider.hpp"
#include "windowUiaProvider.hpp"

#include "../renderer/inc/IRenderData.hpp"
#include "../host/renderData.hpp"
#include "../inc/ServiceLocator.hpp"

using namespace Microsoft::Console::Types;
using namespace Microsoft::Console::Interactivity::Win32;

HRESULT WindowUiaProvider::RuntimeClassInitialize(_In_ IConsoleWindow* baseWindow) noexcept
try
{
    _baseWindow = baseWindow;

    auto& g = ServiceLocator::LocateGlobals();
    auto& gci = g.getConsoleInformation();
    Render::IRenderData* uiaData = &gci.renderData;

    RETURN_IF_FAILED(WRL::MakeAndInitialize<ScreenInfoUiaProvider>(&_pScreenInfoProvider, uiaData, this));

    // TODO GitHub #1914: Re-attach Tracing to UIA Tree
    //Tracing::s_TraceUia(pWindowProvider, ApiCall::Create, nullptr);

    return S_OK;
}
CATCH_RETURN();

[[nodiscard]] HRESULT WindowUiaProvider::Signal(_In_ EVENTID id)
{
    auto hr = S_OK;

    // ScreenInfoUiaProvider is responsible for signaling selection
    // changed events and text changed events
    if (id == UIA_Text_TextSelectionChangedEventId ||
        id == UIA_Text_TextChangedEventId)
    {
        if (_pScreenInfoProvider)
        {
            hr = _pScreenInfoProvider->Signal(id);
        }
        else
        {

Review comment (suggestion):
```suggestion
    Render::IRenderData* renderData = &gci.renderData;
```

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"

#include "screenInfoUiaProvider.hpp"
#include "windowUiaProvider.hpp"

#include "../renderer/inc/IRenderData.hpp"
#include "../host/renderData.hpp"
#include "../inc/ServiceLocator.hpp"

using namespace Microsoft::Console::Types;
using namespace Microsoft::Console::Interactivity::Win32;

HRESULT WindowUiaProvider::RuntimeClassInitialize(_In_ IConsoleWindow* baseWindow) noexcept
try
{
    _baseWindow = baseWindow;

    auto& g = ServiceLocator::LocateGlobals();
    auto& gci = g.getConsoleInformation();
    Render::IRenderData* renderData = &gci.renderData;

    RETURN_IF_FAILED(WRL::MakeAndInitialize<ScreenInfoUiaProvider>(&_pScreenInfoProvider, renderData, this));

    // TODO GitHub #1914: Re-attach Tracing to UIA Tree
    //Tracing::s_TraceUia(pWindowProvider, ApiCall::Create, nullptr);

    return S_OK;
}
CATCH_RETURN();

[[nodiscard]] HRESULT WindowUiaProvider::Signal(_In_ EVENTID id)
{
    auto hr = S_OK;

    // ScreenInfoUiaProvider is responsible for signaling selection
    // changed events and text changed events
    if (id == UIA_Text_TextSelectionChangedEventId ||
        id == UIA_Text_TextChangedEventId)
    {
        if (_pScreenInfoProvider)
        {
            hr = _pScreenInfoProvider->Signal(id);
        }
        else
        {
Repository: microsoft/terminal
File: src/cascadia/TerminalControl/SearchBoxControl.cpp (C++)

Code before:
// Copyright (c) Microsoft Corporation
// Licensed under the MIT license.

#include "pch.h"
#include "SearchBoxControl.h"
#include "SearchBoxControl.g.cpp"
#include <LibraryResources.h>

using namespace winrt;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Core;

namespace winrt::Microsoft::Terminal::Control::implementation
{
    // Constructor
    SearchBoxControl::SearchBoxControl()
    {
        InitializeComponent();

        this->CharacterReceived({ this, &SearchBoxControl::_CharacterHandler });
        this->KeyDown({ this, &SearchBoxControl::_KeyDownHandler });

        auto textBox = TextBox();
        _focusableElements.insert(textBox);
        _focusableElements.insert(CloseButton());
        _focusableElements.insert(CaseSensitivityButton());
        _focusableElements.insert(GoForwardButton());
        _focusableElements.insert(GoBackwardButton());

        // GH#14398: Visible text should match the placeholder text
        Automation::AutomationProperties::SetName(textBox, RS_(L"SearchBox_TextBox/PlaceholderText"));
    }

Review comment (suggestion):
You should only need the `resw` one for this, because it will automatically populate the attached property when it loads the resource tree. If that isn't working, ... then that sucks. Let me know.

Code after:
// Copyright (c) Microsoft Corporation
// Licensed under the MIT license.

#include "pch.h"
#include "SearchBoxControl.h"
#include "SearchBoxControl.g.cpp"

using namespace winrt;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Core;

namespace winrt::Microsoft::Terminal::Control::implementation
{
    // Constructor
    SearchBoxControl::SearchBoxControl()
    {
        InitializeComponent();

        this->CharacterReceived({ this, &SearchBoxControl::_CharacterHandler });
        this->KeyDown({ this, &SearchBoxControl::_KeyDownHandler });

        _focusableElements.insert(TextBox());
        _focusableElements.insert(CloseButton());
        _focusableElements.insert(CaseSensitivityButton());
        _focusableElements.insert(GoForwardButton());
        _focusableElements.insert(GoBackwardButton());
    }

    // Method Description:
    // - Check if the current search direction is forward
    // Arguments:
    // - <none>
Repository: microsoft/terminal
File: src/interactivity/base/InteractivityFactory.cpp (C++)

Code before:
{
    RETURN_HR_IF_NULL(E_INVALIDARG, pVariant);

    pVariant->vt = VT_EMPTY;

    // Returning the default will leave the property as the default
    // so we only really need to touch it for the properties we want to implement
    if (propertyId == UIA_ControlTypePropertyId)
    {
        pVariant->vt = VT_I4;
        pVariant->lVal = UIA_WindowControlTypeId;
    }
    else if (propertyId == UIA_NamePropertyId)
    {
        pVariant->bstrVal = SysAllocString(AutomationPropertyName);
        if (pVariant->bstrVal != nullptr)
        {
            pVariant->vt = VT_BSTR;
        }
    }
    else if (propertyId == UIA_IsControlElementPropertyId)
    {
        pVariant->vt = VT_BOOL;
        pVariant->boolVal = VARIANT_FALSE;
    }
    else if (propertyId == UIA_IsContentElementPropertyId)
    {
        pVariant->vt = VT_BOOL;
        pVariant->boolVal = VARIANT_FALSE;
    }
    else if (propertyId == UIA_IsKeyboardFocusablePropertyId)
    {
        pVariant->vt = VT_BOOL;
        pVariant->boolVal = VARIANT_FALSE;
    }
    else if (propertyId == UIA_HasKeyboardFocusPropertyId)
    {
        pVariant->vt = VT_BOOL;
        pVariant->boolVal = VARIANT_FALSE;
    }
    else if (propertyId == UIA_ProviderDescriptionPropertyId)
    {
        pVariant->bstrVal = SysAllocString(ProviderDescriptionPropertyName);
        if (pVariant->bstrVal != nullptr)
        {
            pVariant->vt = VT_BSTR;
        }
    }
    return S_OK;
}


Review comment (suggestion):
would you mind doing this in a `switch...case`? It will look a lot better when you collapse all the false boolean cases into one body as well...

Code after:
#endif
            default:
                status = STATUS_INVALID_LEVEL;
                break;
            }
        }
        catch (...)
        {
            status = NTSTATUS_FROM_HRESULT(wil::ResultFromCaughtException());
        }

        if (NT_SUCCESS(status))
        {
            control.swap(newControl);
        }
    }

    return status;
}

[[nodiscard]] NTSTATUS InteractivityFactory::CreateConsoleInputThread(_Inout_ std::unique_ptr<IConsoleInputThread>& thread)
{
    auto status = STATUS_SUCCESS;

    ApiLevel level;
    status = ApiDetector::DetectNtUserWindow(&level);

    if (NT_SUCCESS(status))
    {
        std::unique_ptr<IConsoleInputThread> newThread;
        try
        {
            switch (level)
            {
            case ApiLevel::Win32:
                newThread = std::make_unique<Microsoft::Console::Interactivity::Win32::ConsoleInputThread>();
                break;

#ifdef BUILD_ONECORE_INTERACTIVITY
            case ApiLevel::OneCore:
                newThread = std::make_unique<Microsoft::Console::Interactivity::OneCore::ConsoleInputThread>();
                break;
#endif
            default:
                status = STATUS_INVALID_LEVEL;
                break;
            }
        }
        catch (...)
        {
            status = NTSTATUS_FROM_HRESULT(wil::ResultFromCaughtException());