ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/terminal
File: src/host/ft_host/InitTests.cpp (C++)

Code before:

        // Now, try to get at the console we've set up. It's possible 1s wasn't long enough. If that was, we'll try again.

        const auto hOut = GetStdOutputHandle();
        VERIFY_IS_NOT_NULL(hOut, L"Verify we have the standard output handle.");

        CONSOLE_SCREEN_BUFFER_INFOEX csbiexBefore = { 0 };
        csbiexBefore.cbSize = sizeof(csbiexBefore);
        auto succeeded = GetConsoleScreenBufferInfoEx(hOut, &csbiexBefore);
        if (!succeeded)
        {
            auto gle = GetLastError();
            VERIFY_ARE_EQUAL(6u, gle, L"If we fail to set up the console, GetLastError should return 6 here.");
            Sleep(1000);
        }
        else
        {
            break;
        }
    };

    VERIFY_IS_LESS_THAN(tries, 5, L"Make sure we set up the new console in time");

    return true;
}

MODULE_CLEANUP(ModuleCleanup)
{
    v2ModeHelper.reset();
    return true;
}


Review comment (bug):
Another option would be to just `return true` above instead of `break`ing. That way you can blindly `VERIFY_FAIL()` here.

Code after:
        VERIFY_ARE_EQUAL(0, err);

        // Now, try to get at the console we've set up. It's possible 1s wasn't long enough. If that was, we'll try again.

        const auto hOut = GetStdOutputHandle();
        VERIFY_IS_NOT_NULL(hOut, L"Verify we have the standard output handle.");

        CONSOLE_SCREEN_BUFFER_INFOEX csbiexBefore = { 0 };
        csbiexBefore.cbSize = sizeof(csbiexBefore);
        auto succeeded = GetConsoleScreenBufferInfoEx(hOut, &csbiexBefore);
        if (!succeeded)
        {
            const auto gle = GetLastError();
            VERIFY_ARE_EQUAL(6u, gle, L"If we fail to set up the console, GetLastError should return 6 here.");

            // Sleep with a backoff, to give us longer to try next time.
            WaitForSingleObject(GetCurrentThread(), delay);
        }
        else
        {
            Log::Comment(NoThrowString().Format(L"Succeeded on try #%d", tries));
            break;
        }
    };

    VERIFY_IS_LESS_THAN(delay, 30000u, L"Make sure we set up the new console in time");

    return true;
}

MODULE_CLEANUP(ModuleCleanup)
{
    v2ModeHelper.reset();
    return true;
}
Repository: microsoft/terminal
File: src/cascadia/ShellExtension/OpenTerminalHere.cpp (C++)

Code before:
        RETURN_IF_FAILED(psiArray->GetCount(&count));
        if (count) // Sometimes we get an array with a count of 0. Fall back to the site chain.
        {
            RETURN_IF_FAILED(psiArray->GetItemAt(0, psi.put()));
        }
    }

    if (!psi)
    {
        RETURN_IF_FAILED(GetLocationFromSite(psi.put()));
    }

    RETURN_HR_IF(S_FALSE, !psi);
    RETURN_IF_FAILED(psi.copy_to(location));
    return S_OK;
}

// Check is both ctrl and shift keys are pressed during activation of the shell extension
bool OpenTerminalHere::IsControlAndShiftPressed()
{
    short control = 0;
    short shift = 0;
    control = GetAsyncKeyState(VK_CONTROL);
    shift = GetAsyncKeyState(VK_SHIFT);

    //GetAsyncKeyState returns a value with the most significant bit set to 1 if the key is pressed. This is the sign bit.
    return control < 0 && shift < 0;
}


Review comment (suggestion):
```suggestion
    // GetAsyncKeyState returns a value with the most significant bit set to 1 if the key is pressed. This is the sign bit.
```

Code after:
        RETURN_IF_FAILED(psiArray->GetCount(&count));
        if (count) // Sometimes we get an array with a count of 0. Fall back to the site chain.
        {
            RETURN_IF_FAILED(psiArray->GetItemAt(0, psi.put()));
        }
    }

    if (!psi)
    {
        RETURN_IF_FAILED(GetLocationFromSite(psi.put()));
    }

    RETURN_HR_IF(S_FALSE, !psi);
    RETURN_IF_FAILED(psi.copy_to(location));
    return S_OK;
}

// Check is both ctrl and shift keys are pressed during activation of the shell extension
bool OpenTerminalHere::IsControlAndShiftPressed()
{
    short control = 0;
    short shift = 0;
    control = GetAsyncKeyState(VK_CONTROL);
    shift = GetAsyncKeyState(VK_SHIFT);

    // GetAsyncKeyState returns a value with the most significant bit set to 1 if the key is pressed. This is the sign bit.
    return control < 0 && shift < 0;
}
Repository: microsoft/terminal
File: src/cascadia/TerminalApp/CommandPalette.h (C/C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

#include "FilteredCommand.h"
#include "CommandPalette.g.h"
#include "AppCommandlineArgs.h"

struct hash_winrt_object_as_pointer
{
    size_t operator()(const ::winrt::Windows::Foundation::IUnknown& value) const noexcept
    {
        void* const abi_value = winrt::get_abi(value.try_as<::winrt::Windows::Foundation::IUnknown>());
        return std::hash<void*>{}(abi_value);
    }
};

namespace std
{
    template<>
    struct hash<::winrt::Windows::UI::Xaml::DataTemplate> : public hash_winrt_object_as_pointer
    {
    };
    template<>
    struct hash<::winrt::Windows::UI::Xaml::Controls::Primitives::SelectorItem> : public hash_winrt_object_as_pointer
    {
    };
}

// fwdecl unittest classes
namespace TerminalAppLocalTests
{
    class TabTests;
};

namespace winrt::TerminalApp::implementation
{
    enum class CommandPaletteMode
    {
        ActionMode = 0,

Review comment (suggestion):
The extra `try_as` seems redundant? It might make sense to do
```cpp
size_t operator()(const auto& value) const noexcept
{
    return std::hash<void*>{}(winrt::get_abi(value));
}
```

or alternatively
```cpp
size_t operator()(const auto& value) const noexcept
{
    return til::hash(winrt::get_abi(value));
}
```

Due to the novel design choices in `std::unordered_map` I suspect there's not a particularly big difference between the two though, so I don't think it matters what you pick.

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

#include "FilteredCommand.h"
#include "CommandPalette.g.h"
#include "AppCommandlineArgs.h"

#include <til/hash.h>

// fwdecl unittest classes
namespace TerminalAppLocalTests
{
    class TabTests;
};

namespace winrt::TerminalApp::implementation
{
    enum class CommandPaletteMode
    {
        ActionMode = 0,
        TabSearchMode,
        TabSwitchMode,
        CommandlineMode
    };

    struct CommandPalette : CommandPaletteT<CommandPalette>
    {
        CommandPalette();

        Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::FilteredCommand> FilteredActions();

        void SetCommands(const Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::Command>& actions);
        void SetTabs(const Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::TabBase>& tabs, const Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::TabBase>& mruTabs);
        void SetActionMap(const Microsoft::Terminal::Settings::Model::IActionMapView& actionMap);

        bool OnDirectKeyEvent(const uint32_t vkey, const uint8_t scanCode, const bool down);

        void SelectNextItem(const bool moveDown);
Repository: microsoft/terminal
File: src/inc/til/string.h (C/C++)

Code before:
        FAIL_FAST_LAST_ERROR_IF(!result);
        return result;
    }

    struct wstring_case_insensitive_compare
    {
        [[nodiscard]] bool operator()(const std::wstring& lhs, const std::wstring& rhs) const noexcept
        {
            return compare_string_ordinal(lhs, rhs) == CSTR_LESS_THAN;
        }
    };

    // Implement to_int in terms of to_ulong by negating its result. to_ulong does not expect
    // to be passed signed numbers and will return an error accordingly. That error when
    // compared against -1 evaluates to true. We account for that by returning to_int_error if to_ulong
    // returns an error.
    constexpr int to_int(const std::wstring_view& str, unsigned long base = 0) noexcept
    {
        const auto signPosition = str.find(L"-");
        if (signPosition != std::wstring_view::npos)
        {
            const auto result = to_ulong(str.substr(signPosition + 1), base);
            return result == to_ulong_error ? to_int_error : result * -1;
        }

        return to_ulong(str, base) == to_ulong_error ? to_int_error : to_ulong(str, base);
    }
}


Review comment (suggestion):
Awesome! Thanks for doing this.

One remaining concern is that I think we should return `to_int_error` if the resultant value would be outside of the range `INT_MIN .. INT_MAX`

Code after:
        FAIL_FAST_LAST_ERROR_IF(!result);
        return result;
    }

    struct wstring_case_insensitive_compare
    {
        [[nodiscard]] bool operator()(const std::wstring& lhs, const std::wstring& rhs) const noexcept
        {
            return compare_string_ordinal(lhs, rhs) == CSTR_LESS_THAN;
        }
    };

    // Implement to_int in terms of to_ulong by negating its result. to_ulong does not expect
    // to be passed signed numbers and will return an error accordingly. That error when
    // compared against -1 evaluates to true. We account for that by returning to_int_error if to_ulong
    // returns an error.
    constexpr int to_int(const std::wstring_view& str, unsigned long base = 0) noexcept
    {
        auto result = to_ulong_error;
        const auto signPosition = str.find(L"-");
        const bool hasSign = signPosition != std::wstring_view::npos;
        result = hasSign ? to_ulong(str.substr(signPosition + 1), base) : to_ulong(str, base);

        // Check that result is valid and will fit in an int.
        if (result == to_ulong_error || (result > INT_MAX))
        {
            return to_int_error;
        }

        return hasSign ? result * -1 : result;
    }
}
Repository: microsoft/terminal
File: src/types/utils.cpp (C++)

Code before:

    if (isMultiline)
    {
        // In this case, the user totally wanted to paste multiple lines of text,
        // and that likely includes the trailing newline.
        // DON'T trim it in this case.
        return textView;
    }

    return textView.substr(0, lastNonSpace + 1);
}

std::wstring Utils::EvaluateStartingDirectory(
    std::wstring_view currentDirectory,
    std::wstring_view startingDirectory)
{
    std::wstring resultPath{ startingDirectory };

    // We only want to resolve the new WD against the CWD if it doesn't look
    // like a Linux path (see GH#592)

    // Append only if it DOESN'T look like a linux-y path. A linux-y path starts
    // with `~` or `/`.
    const bool looksLikeLinux =
        resultPath.size() >= 1 &&
        (resultPath[0] == L'~' || resultPath[0] == L'/');

    if (!looksLikeLinux)
    {
        std::filesystem::path cwd{ currentDirectory };
        cwd /= startingDirectory;
        resultPath = cwd.wstring();
    }
    return resultPath;
}


Review comment (suggestion):
FYI I wouldn't use `gsl::at`, because it's poorly integrated into MSVC. `til::at` would be sufficient IMO, but `.at(0)` would work just as well.

Code after:

    if (isMultiline)
    {
        // In this case, the user totally wanted to paste multiple lines of text,
        // and that likely includes the trailing newline.
        // DON'T trim it in this case.
        return textView;
    }

    return textView.substr(0, lastNonSpace + 1);
}

std::wstring Utils::EvaluateStartingDirectory(
    std::wstring_view currentDirectory,
    std::wstring_view startingDirectory)
{
    std::wstring resultPath{ startingDirectory };

    // We only want to resolve the new WD against the CWD if it doesn't look
    // like a Linux path (see GH#592)

    // Append only if it DOESN'T look like a linux-y path. A linux-y path starts
    // with `~` or `/`.
    const bool looksLikeLinux =
        resultPath.size() >= 1 &&
        (til::at(resultPath, 0) == L'~' || til::at(resultPath, 0) == L'/');

    if (!looksLikeLinux)
    {
        std::filesystem::path cwd{ currentDirectory };
        cwd /= startingDirectory;
        resultPath = cwd.wstring();
    }
    return resultPath;
}
Repository: microsoft/terminal
File: src/cascadia/WindowsTerminal/WindowThread.h (C/C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
#pragma once
#include "pch.h"
#include "AppHost.h"

class WindowThread : public std::enable_shared_from_this<WindowThread>
{
public:
    WindowThread(winrt::TerminalApp::AppLogic logic,
                 winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args,
                 winrt::Microsoft::Terminal::Remoting::WindowManager manager,
                 winrt::Microsoft::Terminal::Remoting::Peasant peasant);

    winrt::TerminalApp::TerminalWindow Logic();
    void CreateHost();
    int RunMessagePump();
    void RundownForExit();

    winrt::Microsoft::Terminal::Remoting::Peasant Peasant();

    WINRT_CALLBACK(UpdateSettingsRequested, winrt::delegate<void()>);

private:
    winrt::Microsoft::Terminal::Remoting::Peasant _peasant{ nullptr };

    winrt::TerminalApp::AppLogic _appLogic{ nullptr };
    winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs _args{ nullptr };
    winrt::Microsoft::Terminal::Remoting::WindowManager _manager{ nullptr };

    std::unique_ptr<::AppHost> _host{ nullptr };

    int _messagePump();
};


Review comment (suggestion):
I made this change because it avoids exposing the innards of `WindowThread`. It's technically unrelated.

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
#pragma once
#include "pch.h"
#include "AppHost.h"

class WindowThread : public std::enable_shared_from_this<WindowThread>
{
public:
    WindowThread(winrt::TerminalApp::AppLogic logic,
                 winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args,
                 winrt::Microsoft::Terminal::Remoting::WindowManager manager,
                 winrt::Microsoft::Terminal::Remoting::Peasant peasant);

    winrt::TerminalApp::TerminalWindow Logic();
    void CreateHost();
    int RunMessagePump();
    void RundownForExit();

    uint64_t PeasantID();

    WINRT_CALLBACK(UpdateSettingsRequested, winrt::delegate<void()>);

private:
    winrt::Microsoft::Terminal::Remoting::Peasant _peasant{ nullptr };

    winrt::TerminalApp::AppLogic _appLogic{ nullptr };
    winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs _args{ nullptr };
    winrt::Microsoft::Terminal::Remoting::WindowManager _manager{ nullptr };

    std::unique_ptr<::AppHost> _host{ nullptr };

    int _messagePump();
};
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/PreviewConnection.cpp (C++)

Code before:
        _TerminalOutputHandlers(fmt::format(PreviewText, _displayPowerlineGlyphs ? PromptTextPowerline : PromptTextPlain));
    }

    void PreviewConnection::Initialize(const Windows::Foundation::Collections::ValueSet& /*settings*/) noexcept
    {
    }

    void PreviewConnection::WriteInput(const hstring& /*data*/)
    {
    }

    void PreviewConnection::Resize(uint32_t /*rows*/, uint32_t /*columns*/) noexcept
    {
    }

    void PreviewConnection::Close() noexcept
    {
    }

    void PreviewConnection::DisplayPowerlineGlyphs(bool d) noexcept
    {
        std::swap(d, _displayPowerlineGlyphs);
        if (_displayPowerlineGlyphs != d)
        {
            // If it changed, redraw
            Start();
        }
    }
}


Review comment (suggestion):
The more classical approach would be (I think the comment is redundant btw):
```suggestion
        if (_displayPowerlineGlyphs != d)
        {
            _displayPowerlineGlyphs = d;
            Start();
        }
```

Code after:
        _TerminalOutputHandlers(fmt::format(PreviewText, _displayPowerlineGlyphs ? PromptTextPowerline : PromptTextPlain));
    }

    void PreviewConnection::Initialize(const Windows::Foundation::Collections::ValueSet& /*settings*/) noexcept
    {
    }

    void PreviewConnection::WriteInput(const hstring& /*data*/)
    {
    }

    void PreviewConnection::Resize(uint32_t /*rows*/, uint32_t /*columns*/) noexcept
    {
    }

    void PreviewConnection::Close() noexcept
    {
    }

    void PreviewConnection::DisplayPowerlineGlyphs(bool d) noexcept
    {
        if (_displayPowerlineGlyphs != d)
        {
            _displayPowerlineGlyphs = d;
            Start();
        }
    }
}
Repository: microsoft/terminal
File: src/buffer/out/textBuffer.hpp (C++)

Code before:
ScreenInfo->Rows points to the ROW array. ScreenInfo->Rows[0] is not
necessarily the top row. ScreenInfo->BufferInfo.TextInfo->FirstRow contains the index of
the top row.  That means scrolling (if scrolling entire screen)
merely involves changing the FirstRow index,
filling in the last row, and updating the screen.

--*/

#pragma once

#include <vector>

#include "cursor.h"
#include "Row.hpp"
#include "TextAttribute.hpp"
#include "../types/inc/Viewport.hpp"

#include "../buffer/out/textBufferCellIterator.hpp"
#include "../buffer/out/textBufferTextIterator.hpp"

namespace Microsoft::Console::Render
{
    class Renderer;
}

enum class MarkCategory : size_t
{
    Prompt = 0,
    Error = 1,
    Warning = 2,
    Success = 3,
    Info = 4
};
struct ScrollMark
{
    std::optional<til::color> color;
    til::point start;
    til::point end; // exclusive
    std::optional<til::point> commandEnd;
    std::optional<til::point> outputEnd;

    MarkCategory category{ MarkCategory::Info };
    // Other things we may want to think about in the future are listed in
    // GH#11000

    bool HasCommand() const noexcept
    {
        return commandEnd.has_value() && *commandEnd != end;
    }
    bool HasOutput() const noexcept
    {

Review comment (suggestion):
FYI you can use `uint8_t` here or just remove the explicit underlying type entirely.

Code after:
ScreenInfo->Rows points to the ROW array. ScreenInfo->Rows[0] is not
necessarily the top row. ScreenInfo->BufferInfo.TextInfo->FirstRow contains the index of
the top row.  That means scrolling (if scrolling entire screen)
merely involves changing the FirstRow index,
filling in the last row, and updating the screen.

--*/

#pragma once

#include <vector>

#include "cursor.h"
#include "Row.hpp"
#include "TextAttribute.hpp"
#include "../types/inc/Viewport.hpp"

#include "../buffer/out/textBufferCellIterator.hpp"
#include "../buffer/out/textBufferTextIterator.hpp"

namespace Microsoft::Console::Render
{
    class Renderer;
}

enum class MarkCategory
{
    Prompt = 0,
    Error = 1,
    Warning = 2,
    Success = 3,
    Info = 4
};
struct ScrollMark
{
    std::optional<til::color> color;
    til::point start;
    til::point end; // exclusive
    std::optional<til::point> commandEnd;
    std::optional<til::point> outputEnd;

    MarkCategory category{ MarkCategory::Info };
    // Other things we may want to think about in the future are listed in
    // GH#11000

    bool HasCommand() const noexcept
    {
        return commandEnd.has_value() && *commandEnd != end;
    }
    bool HasOutput() const noexcept
    {
Repository: microsoft/terminal
File: src/host/history.cpp (C++)

Code before:
            {
                _Reset();
            }
        }
    }
    CATCH_RETURN();
    WI_SetFlag(Flags, CLE_RESET); // remember that we've returned a cmd

    return S_OK;
}

std::wstring_view CommandHistory::GetNth(Index index) const
{
    if (index >= 0 && index < _commands.size())
    {
        return _commands.at(index);
    }
    return {};
}

const std::vector<std::wstring>& CommandHistory::GetCommands() const noexcept
{
    return _commands;
}

[[nodiscard]] HRESULT CommandHistory::RetrieveNth(const Index index, std::span<wchar_t> buffer, size_t& commandSize)
{
    LastDisplayed = index;

    try
    {
        const auto& cmd = _commands.at(index);
        if (cmd.size() > buffer.size())
        {
            commandSize = buffer.size(); // room for CRLF?
        }
        else
        {
            commandSize = cmd.size();
        }

        std::copy_n(cmd.cbegin(), commandSize, buffer.begin());

        commandSize *= sizeof(wchar_t);

        return S_OK;
    }
    CATCH_RETURN();
}

[[nodiscard]] HRESULT CommandHistory::Retrieve(const SearchDirection searchDirection,

Review comment (suggestion):
Ugh we have Get and Retrieve. Can we rename `Retrieve` to `Copy`? They're both synonyms otherwise, and I hate methods whose names are synonyms but behaviors are totally unrelated.

ALSO THIS UPDATES STATE WHERE GET DOESN'T.
AGH.

Code after:
            {
                _Reset();
            }
        }
    }
    CATCH_RETURN();
    WI_SetFlag(Flags, CLE_RESET); // remember that we've returned a cmd

    return S_OK;
}

std::wstring_view CommandHistory::GetNth(Index index) const
{
    if (index >= 0 && index < GetNumberOfCommands())
    {
        return _commands.at(index);
    }
    return {};
}

const std::vector<std::wstring>& CommandHistory::GetCommands() const noexcept
{
    return _commands;
}

[[nodiscard]] HRESULT CommandHistory::RetrieveNth(const Index index, std::span<wchar_t> buffer, size_t& commandSize)
{
    LastDisplayed = index;

    try
    {
        const auto& cmd = _commands.at(index);
        if (cmd.size() > buffer.size())
        {
            commandSize = buffer.size(); // room for CRLF?
        }
        else
        {
            commandSize = cmd.size();
        }

        std::copy_n(cmd.cbegin(), commandSize, buffer.begin());

        commandSize *= sizeof(wchar_t);

        return S_OK;
    }
    CATCH_RETURN();
}

[[nodiscard]] HRESULT CommandHistory::Retrieve(const SearchDirection searchDirection,