ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsModel/FontConfig.cpp (C++)

Code before:
//   will be set to nullopt.
// - This is similar to Profile::LayerJson but for FontConfig
// Arguments:
// - json: an object which should be a partial serialization of a FontConfig object.
void FontConfig::LayerJson(const Json::Value& json)
{
    // Legacy users may not have a font object defined in their profile,
    // so check for that before we decide how to parse this
    if (json.isMember(JsonKey(FontInfoKey)))
    {
        // A font object is defined, use that
        const auto fontInfoJson = json[JsonKey(FontInfoKey)];
        JsonUtils::GetValueForKey(fontInfoJson, FontFaceKey, _FontFace);
        JsonUtils::GetValueForKey(fontInfoJson, FontSizeKey, _FontSize);
        JsonUtils::GetValueForKey(fontInfoJson, FontWeightKey, _FontWeight);
    }
    else
    {
        // No font object is defined
        JsonUtils::GetValueForKey(json, LegacyFontFaceKey, _FontFace);
        JsonUtils::GetValueForKey(json, LegacyFontSizeKey, _FontSize);
        JsonUtils::GetValueForKey(json, LegacyFontWeightKey, _FontWeight);
    }
}

bool FontConfig::HasAnyOptionSet()
{
    return HasFontFace() || HasFontSize() || HasFontWeight();
}

winrt::Microsoft::Terminal::Settings::Model::Profile FontConfig::SourceProfile()
{
    return _sourceProfile.get();
}


Review comment (suggestion):
```suggestion
bool FontConfig::HasAnyOptionSet() const
```

Code after:
// - This is similar to Profile::LayerJson but for FontConfig
// Arguments:
// - json: an object which should be a partial serialization of a FontConfig object.
void FontConfig::LayerJson(const Json::Value& json)
{
    // Legacy users may not have a font object defined in their profile,
    // so check for that before we decide how to parse this
    if (json.isMember(JsonKey(FontInfoKey)))
    {
        // A font object is defined, use that
        const auto fontInfoJson = json[JsonKey(FontInfoKey)];
        JsonUtils::GetValueForKey(fontInfoJson, FontFaceKey, _FontFace);
        JsonUtils::GetValueForKey(fontInfoJson, FontSizeKey, _FontSize);
        JsonUtils::GetValueForKey(fontInfoJson, FontWeightKey, _FontWeight);
    }
    else
    {
        // No font object is defined
        JsonUtils::GetValueForKey(json, LegacyFontFaceKey, _FontFace);
        JsonUtils::GetValueForKey(json, LegacyFontSizeKey, _FontSize);
        JsonUtils::GetValueForKey(json, LegacyFontWeightKey, _FontWeight);
    }
}

bool FontConfig::HasAnyOptionSet() const
{
    return HasFontFace() || HasFontSize() || HasFontWeight();
}

winrt::Microsoft::Terminal::Settings::Model::Profile FontConfig::SourceProfile()
{
    return _sourceProfile.get();
}
Repository: microsoft/terminal
File: src/cascadia/WinRTUtils/inc/WtExeUtils.h (C/C++)

Code before:
        // `wt.exe` on the %PATH% is us or not. Fortunately, _our_ execution alias
        // is located in "%LOCALAPPDATA%\Microsoft\WindowsApps\<our package family
        // name>", _always_, so we can use that to look up the exe easier.
        if (IsPackaged())
        {
            try
            {
                const auto package = winrt::Windows::ApplicationModel::Package::Current();
                const auto id = package.Id();
                const auto pfn = id.FamilyName();
                if (!pfn.empty())
                {
                    const std::filesystem::path windowsAppsPath{ wil::ExpandEnvironmentStringsW<std::wstring>(LocalAppDataAppsPath.data()) };
                    const std::filesystem::path wtPath = windowsAppsPath / std::wstring_view{ pfn } / (IsDevBuild() ? WtdExe : WtExe);
                    return wtPath;
                }
            }
            CATCH_LOG();
        }

        // If we're here, then we couldn't resolve our exe from the package. This
        // means we're running unpackaged. We should just use the
        // WindowsTerminal.exe that's sitting in the directory next to us.
        try
        {
            std::filesystem::path module = wil::GetModuleFileNameW<std::wstring>(nullptr);
            module.replace_filename(WindowsTerminalExe);
            return module;
        }
        CATCH_LOG();

        return std::wstring{ WtExe };
    }();
    return exePath;
}


Review comment (suggestion):
This code improvement stems from `_getExePath`. I then removed `_getExePath` as it was an almost 1:1 copy of this function.

Code after:
        // name>", _always_, so we can use that to look up the exe easier.
        if (IsPackaged())
        {
            try
            {
                const auto package = winrt::Windows::ApplicationModel::Package::Current();
                const auto id = package.Id();
                const auto pfn = id.FamilyName();
                if (!pfn.empty())
                {
                    const std::filesystem::path windowsAppsPath{ wil::ExpandEnvironmentStringsW<std::wstring>(LocalAppDataAppsPath.data()) };
                    const std::filesystem::path wtPath = windowsAppsPath / std::wstring_view{ pfn } / (IsDevBuild() ? WtdExe : WtExe);
                    return wtPath;
                }
            }
            CATCH_LOG();
        }

        // If we're here, then we couldn't resolve our exe from the package. This
        // means we're running unpackaged. We should just use the
        // WindowsTerminal.exe that's sitting in the directory next to us.
        try
        {
            std::filesystem::path module = wil::GetModuleFileNameW<std::wstring>(nullptr);
            module.replace_filename(WindowsTerminalExe);
            return module;
        }
        CATCH_LOG();

        return std::wstring{ WtExe };
    }();
    return exePath;
}
Repository: microsoft/terminal
File: src/cascadia/WinRTUtils/inc/WtExeUtils.h (C/C++)

Code before:
        {
            try
            {
                const auto package = winrt::Windows::ApplicationModel::Package::Current();
                const auto id = package.Id();
                const auto pfn = id.FamilyName();
                if (!pfn.empty())
                {
                    const std::filesystem::path windowsAppsPath{ wil::ExpandEnvironmentStringsW<std::wstring>(LocalAppDataAppsPath.data()) };
                    const std::filesystem::path wtPath = windowsAppsPath / std::wstring_view{ pfn } / (IsDevBuild() ? WtdExe : WtExe);
                    return wtPath;
                }
            }
            CATCH_LOG();
        }

        // If we're here, then we couldn't resolve our exe from the package. This
        // means we're running unpackaged. We should just use the
        // WindowsTerminal.exe that's sitting in the directory next to us.
        try
        {
            std::filesystem::path module = wil::GetModuleFileNameW<std::wstring>(nullptr);
            module.replace_filename(WindowsTerminalExe);
            return module;
        }
        CATCH_LOG();

        return std::wstring{ WtExe };
    }();
    return exePath;
}


Review comment (suggestion):
This was really confusing for me... It just wouldn't compile without explicitly specifying the type again, because it was complaining about the constructor being explicit.

Code after:
            {
                const auto package = winrt::Windows::ApplicationModel::Package::Current();
                const auto id = package.Id();
                const auto pfn = id.FamilyName();
                if (!pfn.empty())
                {
                    const std::filesystem::path windowsAppsPath{ wil::ExpandEnvironmentStringsW<std::wstring>(LocalAppDataAppsPath.data()) };
                    const std::filesystem::path wtPath = windowsAppsPath / std::wstring_view{ pfn } / (IsDevBuild() ? WtdExe : WtExe);
                    return wtPath;
                }
            }
            CATCH_LOG();
        }

        // If we're here, then we couldn't resolve our exe from the package. This
        // means we're running unpackaged. We should just use the
        // WindowsTerminal.exe that's sitting in the directory next to us.
        try
        {
            std::filesystem::path module = wil::GetModuleFileNameW<std::wstring>(nullptr);
            module.replace_filename(WindowsTerminalExe);
            return module;
        }
        CATCH_LOG();

        return std::wstring{ WtExe };
    }();
    return exePath;
}
Repository: microsoft/terminal
File: src/terminal/parser/stateMachine.hpp (C++)

Code before:
            DcsEntry,
            DcsIgnore,
            DcsIntermediate,
            DcsParam,
            DcsPassThrough,
            SosPmApcString
        };

        Microsoft::Console::VirtualTerminal::ParserTracing _trace;

        std::unique_ptr<IStateMachineEngine> _engine;

        VTStates _state;

        bool _isInAnsiMode;

        std::wstring_view _run;
        std::wstring_view _currentString;

        size_t _runOffset;
        size_t _runSize;

        void _ActuateCurrentRun()
        {
            _run = _currentString.substr(_runOffset, _runSize);
        }

        VTIDBuilder _identifier;
        std::vector<VTParameter> _parameters;
        bool _parameterLimitReached;

        std::wstring _oscString;
        size_t _oscParameter;

        IStateMachineEngine::StringHandler _dcsStringHandler;

        std::optional<std::wstring> _cachedSequence;

        // This is tracked per state machine instance so that separate calls to Process*
        //   can start and finish a sequence.
        bool _processingIndividually;
    };
}


Review comment (suggestion):
If you change this to:
```cpp
std::string_view _CurrentRun() const {
    return _currentString.substr(_runOffset, _runSize);
}
```

we can do the other optimizations I'm mentioning.

Code after:
            DcsEntry,
            DcsIgnore,
            DcsIntermediate,
            DcsParam,
            DcsPassThrough,
            SosPmApcString
        };

        Microsoft::Console::VirtualTerminal::ParserTracing _trace;

        std::unique_ptr<IStateMachineEngine> _engine;

        VTStates _state;

        bool _isInAnsiMode;

        std::wstring_view _currentString;
        size_t _runOffset;
        size_t _runSize;

        // Construct current run.
        //
        // Note: We intentionally use this method to create the run lazily for better performance.
        //       You may find the usage of offset & size unsafe, but under heavy load it shows noticeable performance benefit.
        std::wstring_view _CurrentRun() const
        {
            return _currentString.substr(_runOffset, _runSize);
        }

        VTIDBuilder _identifier;
        std::vector<VTParameter> _parameters;
        bool _parameterLimitReached;

        std::wstring _oscString;
        size_t _oscParameter;

        IStateMachineEngine::StringHandler _dcsStringHandler;

        std::optional<std::wstring> _cachedSequence;

        // This is tracked per state machine instance so that separate calls to Process*
        //   can start and finish a sequence.
        bool _processingIndividually;
    };
}
Repository: microsoft/terminal
File: src/terminal/parser/stateMachine.hpp (C++)

Code before:
            Vt52Param,
            DcsEntry,
            DcsIgnore,
            DcsIntermediate,
            DcsParam,
            DcsPassThrough,
            SosPmApcString
        };

        Microsoft::Console::VirtualTerminal::ParserTracing _trace;

        std::unique_ptr<IStateMachineEngine> _engine;

        VTStates _state;

        bool _isInAnsiMode;

        std::wstring_view _currentString;
        size_t _runOffset;
        size_t _runSize;

        std::wstring_view _CurrentRun() const
        {
            return _currentString.substr(_runOffset, _runSize);
        }

        VTIDBuilder _identifier;
        std::vector<VTParameter> _parameters;
        bool _parameterLimitReached;

        std::wstring _oscString;
        size_t _oscParameter;

        IStateMachineEngine::StringHandler _dcsStringHandler;

        std::optional<std::wstring> _cachedSequence;

        // This is tracked per state machine instance so that separate calls to Process*
        //   can start and finish a sequence.
        bool _processingIndividually;
    };
}


Review comment (refactor):
Maybe leave some commentary as to why this is the way that it is to prevent future refactoring that makes the perf worse.

Code after:
            Vt52Param,
            DcsEntry,
            DcsIgnore,
            DcsIntermediate,
            DcsParam,
            DcsPassThrough,
            SosPmApcString
        };

        Microsoft::Console::VirtualTerminal::ParserTracing _trace;

        std::unique_ptr<IStateMachineEngine> _engine;

        VTStates _state;

        bool _isInAnsiMode;

        std::wstring_view _currentString;
        size_t _runOffset;
        size_t _runSize;

        // Construct current run.
        //
        // Note: We intentionally use this method to create the run lazily for better performance.
        //       You may find the usage of offset & size unsafe, but under heavy load it shows noticeable performance benefit.
        std::wstring_view _CurrentRun() const
        {
            return _currentString.substr(_runOffset, _runSize);
        }

        VTIDBuilder _identifier;
        std::vector<VTParameter> _parameters;
        bool _parameterLimitReached;

        std::wstring _oscString;
        size_t _oscParameter;

        IStateMachineEngine::StringHandler _dcsStringHandler;

        std::optional<std::wstring> _cachedSequence;

        // This is tracked per state machine instance so that separate calls to Process*
        //   can start and finish a sequence.
        bool _processingIndividually;
    };
}
Repository: microsoft/terminal
File: src/api-ms-win-core-synch-l1-2-0/main.cpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

//
// The code in this file was adapted from the STL on the 2021-07-05. Commit:
// https://github.com/microsoft/STL/blob/e745bad3b1d05b5b19ec652d68abb37865ffa454/stl/src/atomic_wait.cpp
//
// It backports the following Windows 8 functions to Windows 7:
// * WaitOnAddress
// * WakeByAddressSingle
// * WakeByAddressAll
//

#include <cstdint>
#include <new>

#include <winsdkver.h>
#define _WIN32_WINNT 0x0601
#include <sdkddkver.h>

#define WIN32_LEAN_AND_MEAN
#define NOMINMAX
#include <Windows.h>
#include <intrin.h>

namespace
{
    class [[nodiscard]] SRWLockGuard
    {
    public:
        explicit SRWLockGuard(SRWLOCK & lock) noexcept :
            lock(&lock)
        {
            AcquireSRWLockExclusive(&lock);
        }

        ~SRWLockGuard()
        {
            ReleaseSRWLockExclusive(lock);
        }

        SRWLockGuard(const SRWLockGuard&) = delete;
        SRWLockGuard& operator=(const SRWLockGuard&) = delete;

    private:
        SRWLOCK* lock;
    };

Review comment (suggestion):
`NOMINMAX` should be included in `WIN32_LEAN_AND_MEAN`.  

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

//
// The code in this file was adapted from the STL on the 2021-07-05. Commit:
// https://github.com/microsoft/STL/blob/e745bad3b1d05b5b19ec652d68abb37865ffa454/stl/src/atomic_wait.cpp
//
// It backports the following Windows 8 functions to Windows 7:
// * WaitOnAddress
// * WakeByAddressSingle
// * WakeByAddressAll
//

#include <cstdint>
#include <new>

#include <winsdkver.h>
#define _WIN32_WINNT 0x0601
#include <sdkddkver.h>

#define WIN32_LEAN_AND_MEAN
#define NOMINMAX
#include <Windows.h>
#include <intrin.h>

namespace
{
    class [[nodiscard]] SRWLockGuard
    {
    public:
        explicit SRWLockGuard(SRWLOCK & lock) noexcept :
            _lock(&lock)
        {
            AcquireSRWLockExclusive(_lock);
        }

        ~SRWLockGuard()
        {
            ReleaseSRWLockExclusive(_lock);
        }

        SRWLockGuard(const SRWLockGuard&) = delete;
        SRWLockGuard& operator=(const SRWLockGuard&) = delete;

        SRWLockGuard(SRWLockGuard &&) = delete;
        SRWLockGuard& operator=(SRWLockGuard&&) = delete;
Repository: microsoft/terminal
File: src/interactivity/base/ServiceLocator.cpp (C++)

Code before:
    }
    else
    {
        if (s_interactivityFactory.get() == nullptr)
        {
            status = ServiceLocator::LoadInteractivityFactory();
        }
        if (NT_SUCCESS(status))
        {
            status = s_interactivityFactory->CreateConsoleInputThread(s_consoleInputThread);

            if (NT_SUCCESS(status))
            {
                *thread = s_consoleInputThread.get();
            }
        }
    }

    return status;
}

[[nodiscard]] HRESULT ServiceLocator::CreateAccessibilityNotifier(const bool isPty)
{
    if (isPty)
    {
        return S_FALSE;
    }

    // Can't create if we've already created.
    if (s_accessibilityNotifier)
    {
        return E_UNEXPECTED;
    }

    if (!s_interactivityFactory)
    {
        RETURN_IF_NTSTATUS_FAILED(ServiceLocator::LoadInteractivityFactory());
    }

    RETURN_IF_NTSTATUS_FAILED(s_interactivityFactory->CreateAccessibilityNotifier(s_accessibilityNotifier));

    return S_OK;
}

#pragma endregion

#pragma region Set Methods

[[nodiscard]] NTSTATUS ServiceLocator::SetConsoleControlInstance(_In_ std::unique_ptr<IConsoleControl>&& control)
{
    if (s_consoleControl)

Review comment (bug):
If this returns `S_FALSE`, doesn't the caller return a failure when it checks with `RETURN_IF_FAILED`? Do we want that?

Code after:
    }
    else
    {
        if (s_interactivityFactory.get() == nullptr)
        {
            status = ServiceLocator::LoadInteractivityFactory();
        }
        if (NT_SUCCESS(status))
        {
            status = s_interactivityFactory->CreateConsoleInputThread(s_consoleInputThread);

            if (NT_SUCCESS(status))
            {
                *thread = s_consoleInputThread.get();
            }
        }
    }

    return status;
}

[[nodiscard]] HRESULT ServiceLocator::CreateAccessibilityNotifier()
{
    // Can't create if we've already created.
    if (s_accessibilityNotifier)
    {
        return E_UNEXPECTED;
    }

    if (!s_interactivityFactory)
    {
        RETURN_IF_NTSTATUS_FAILED(ServiceLocator::LoadInteractivityFactory());
    }

    RETURN_IF_NTSTATUS_FAILED(s_interactivityFactory->CreateAccessibilityNotifier(s_accessibilityNotifier));

    return S_OK;
}

#pragma endregion

#pragma region Set Methods

[[nodiscard]] NTSTATUS ServiceLocator::SetConsoleControlInstance(_In_ std::unique_ptr<IConsoleControl>&& control)
{
    if (s_consoleControl)
    {
        NT_RETURN_NTSTATUS(STATUS_INVALID_HANDLE);
    }
    else if (!control)
    {
Repository: microsoft/terminal
File: src/renderer/dx/DxRenderer.cpp (C++)

Code before:
{
    if (_softwareRendering != enable)
    {
        _softwareRendering = enable;
        _recreateDeviceRequested = true;
        LOG_IF_FAILED(InvalidateAll());
    }
}
CATCH_LOG()

HANDLE DxEngine::GetSwapChainHandle()
{
    if (!_swapChainHandle)
    {
        THROW_IF_FAILED(_CreateDeviceResources(true));
    }

    return _swapChainHandle.get();
}

void DxEngine::_InvalidateRectangle(const til::rectangle& rc)
{
    auto invalidate = rc;

    if (_invalidateFullRows)
    {
        invalidate = til::rectangle{ til::point{ static_cast<ptrdiff_t>(0), rc.top() }, til::size{ _invalidMap.size().width(), rc.height() } };
    }

    _invalidMap.set(invalidate);
}

bool DxEngine::_IsAllInvalid() const noexcept
{
    return std::llabs(_invalidScroll.y()) >= _invalidMap.size().height();
}

// Routine Description:
// - Invalidates a rectangle described in characters
// Arguments:
// - psrRegion - Character rectangle
// Return Value:
// - S_OK
[[nodiscard]] HRESULT DxEngine::Invalidate(const SMALL_RECT* const psrRegion) noexcept
try
{
    RETURN_HR_IF_NULL(E_INVALIDARG, psrRegion);

    if (!_allInvalid)
    {
        _InvalidateRectangle(Viewport::FromExclusive(*psrRegion).ToInclusive());

Review comment (suggestion):
Due to the missing `std::min`, but the delayed `_invalidMap` resize, this was blowing up with exceptions. Now it isn't anymore.

Code after:
{
    if (_softwareRendering != enable)
    {
        _softwareRendering = enable;
        _recreateDeviceRequested = true;
        LOG_IF_FAILED(InvalidateAll());
    }
}
CATCH_LOG()

HANDLE DxEngine::GetSwapChainHandle()
{
    if (!_swapChainHandle)
    {
        THROW_IF_FAILED(_CreateDeviceResources(true));
    }

    return _swapChainHandle.get();
}

void DxEngine::_InvalidateRectangle(const til::rectangle& rc)
{
    const auto size = _invalidMap.size();
    const auto topLeft = til::point{ 0, std::min(size.height(), rc.top()) };
    const auto bottomRight = til::point{ size.width(), std::min(size.height(), rc.bottom()) };
    _invalidMap.set({ topLeft, bottomRight });
}

bool DxEngine::_IsAllInvalid() const noexcept
{
    return std::llabs(_invalidScroll.y()) >= _invalidMap.size().height();
}

// Routine Description:
// - Invalidates a rectangle described in characters
// Arguments:
// - psrRegion - Character rectangle
// Return Value:
// - S_OK
[[nodiscard]] HRESULT DxEngine::Invalidate(const SMALL_RECT* const psrRegion) noexcept
try
{
    RETURN_HR_IF_NULL(E_INVALIDARG, psrRegion);

    if (!_allInvalid)
    {
        _InvalidateRectangle(Viewport::FromExclusive(*psrRegion).ToInclusive());
    }

    return S_OK;
}
Repository: microsoft/terminal
File: src/cascadia/WindowsTerminal/TrayIcon.h (C/C++)

Code before:
};

class TrayIcon
{
public:
    TrayIcon() = delete;
    TrayIcon(const HWND owningHwnd);
    ~TrayIcon();

    void CreateWindowProcess();
    static LRESULT CALLBACK _WindowProc(HWND window, UINT message, WPARAM wparam, LPARAM lparam) noexcept;

    void CreateTrayIcon();
    void RemoveIconFromTray();
    void ReAddTrayIcon();

    void TrayIconPressed();
    void ShowTrayContextMenu(const til::point coord, winrt::Windows::Foundation::Collections::IMapView<uint64_t, winrt::hstring> peasants);
    void TrayMenuItemSelected(const HMENU menu, const UINT menuItemIndex);

    WINRT_CALLBACK(SummonWindowRequested, winrt::delegate<void(winrt::Microsoft::Terminal::Remoting::SummonWindowSelectionArgs)>);

private:
    HMENU _CreateTrayContextMenu(winrt::Windows::Foundation::Collections::IMapView<uint64_t, winrt::hstring> peasants);

    HWND _trayIconWndProc;
    HWND _owningHwnd;
    NOTIFYICONDATA _trayIconData;
};


Review comment (suggestion):
naming this `WndProc` seems weird - makes me thing it's a handle to the actual wndproc function itself. maybe just 
```suggestion
    HWND _trayIconHwnd;
```

Code after:
};

class TrayIcon
{
public:
    TrayIcon() = delete;
    TrayIcon(const HWND owningHwnd);
    ~TrayIcon();

    void CreateTrayIcon();
    void RemoveIconFromTray();
    void ReAddTrayIcon();

    void TrayIconPressed();
    void ShowTrayContextMenu(const til::point coord, winrt::Windows::Foundation::Collections::IMapView<uint64_t, winrt::hstring> peasants);
    void TrayMenuItemSelected(const HMENU menu, const UINT menuItemIndex);

    WINRT_CALLBACK(SummonWindowRequested, winrt::delegate<void(winrt::Microsoft::Terminal::Remoting::SummonWindowSelectionArgs)>);

private:
    void _CreateWindow();
    HMENU _CreateTrayContextMenu(winrt::Windows::Foundation::Collections::IMapView<uint64_t, winrt::hstring> peasants);

    wil::unique_hwnd _trayIconHwnd;
    HWND _owningHwnd;
    NOTIFYICONDATA _trayIconData;
};