ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/terminal
File: src/cascadia/WindowsTerminal/WindowThread.h (C/C++)

Code before:
    bool KeepWarm();
    void Refrigerate();
    void Microwave(
        winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args,
        winrt::Microsoft::Terminal::Remoting::Peasant peasant);

    uint64_t PeasantID();

    til::event<winrt::delegate<void()>> UpdateSettingsRequested;

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 };

    // This is a "shared_ptr", but it should be treated as a unique, owning ptr.
    // It's shared, because there are edge cases in refrigeration where internal
    // co_awaits inside AppHost might resume after we've dtor'd it, and there's
    // no other way for us to let the AppHost know it has passed on.
    std::shared_ptr<::AppHost> _host{ nullptr };
    winrt::event_token _UpdateSettingsRequestedToken;

    std::unique_ptr<::IslandWindow> _warmWindow{ nullptr };

    int _messagePump();
    void _pumpRemainingXamlMessages();
};


Review comment (suggestion):
You don't need to attach statics to classes. It's just fine to have mutable statics inside functions. If they're of trivial types (bool, int, float, etc.) there's no difference. (For complex types there's a difference: Class statics get initialized like globals on startup and function statics get initialized the first time execution passes over them.)

Code after:
    bool KeepWarm();
    void Refrigerate();
    void Microwave(
        winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args,
        winrt::Microsoft::Terminal::Remoting::Peasant peasant);

    uint64_t PeasantID();

    til::event<winrt::delegate<void()>> UpdateSettingsRequested;

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 };

    // This is a "shared_ptr", but it should be treated as a unique, owning ptr.
    // It's shared, because there are edge cases in refrigeration where internal
    // co_awaits inside AppHost might resume after we've dtor'd it, and there's
    // no other way for us to let the AppHost know it has passed on.
    std::shared_ptr<::AppHost> _host{ nullptr };
    winrt::event_token _UpdateSettingsRequestedToken;

    std::unique_ptr<::IslandWindow> _warmWindow{ nullptr };
    static bool _loggedInteraction;

    int _messagePump();
    void _pumpRemainingXamlMessages();
};
Repository: microsoft/terminal
File: src/interactivity/win32/uiaTextRange.cpp (C++)

Code before:

HRESULT UiaTextRange::RuntimeClassInitialize(const UiaTextRange& a)
{
    return UiaTextRangeBase::RuntimeClassInitialize(a);
}

IFACEMETHODIMP UiaTextRange::Clone(_Outptr_result_maybenull_ ITextRangeProvider** ppRetVal)
{
    RETURN_HR_IF(E_INVALIDARG, ppRetVal == nullptr);
    *ppRetVal = nullptr;
    RETURN_IF_FAILED(MakeAndInitialize<UiaTextRange>(ppRetVal, *this));

    // TODO GitHub #1914: Re-attach Tracing to UIA Tree
    // tracing
    /*ApiMsgClone apiMsg;
    apiMsg.CloneId = static_cast<UiaTextRangeBase*>(*ppRetVal)->GetId();
    Tracing::s_TraceUia(this, ApiCall::Clone, &apiMsg);*/

    return S_OK;
}

void UiaTextRange::_TranslatePointToScreen(til::point& clientPoint) const
{
    auto win32Point = clientPoint.as_win32_point();
    ClientToScreen(_getWindowHandle(), win32Point);
    clientPoint = til::point{ *win32Point };
}

void UiaTextRange::_TranslatePointFromScreen(til::point& screenPoint) const
{
    auto win32Point = screenPoint.as_win32_point();
    ScreenToClient(_getWindowHandle(), win32Point);
    screenPoint = til::point{ *win32Point };
}

HWND UiaTextRange::_getWindowHandle() const
{
    const auto provider = static_cast<ScreenInfoUiaProvider*>(_pProvider);
    return provider->GetWindowHandle();
}


Review comment (suggestion):
honestly I'd revert this. It's actually not doing what you think - since you have a reference and get a pointer to it, `ClientToScreen` is _still_ directly overwriting `clientPoint`. Better to skip the middleman and stick with the code before this commit. :D

Code after:

HRESULT UiaTextRange::RuntimeClassInitialize(const UiaTextRange& a)
{
    return UiaTextRangeBase::RuntimeClassInitialize(a);
}

IFACEMETHODIMP UiaTextRange::Clone(_Outptr_result_maybenull_ ITextRangeProvider** ppRetVal)
{
    RETURN_HR_IF(E_INVALIDARG, ppRetVal == nullptr);
    *ppRetVal = nullptr;
    RETURN_IF_FAILED(MakeAndInitialize<UiaTextRange>(ppRetVal, *this));

    // TODO GitHub #1914: Re-attach Tracing to UIA Tree
    // tracing
    /*ApiMsgClone apiMsg;
    apiMsg.CloneId = static_cast<UiaTextRangeBase*>(*ppRetVal)->GetId();
    Tracing::s_TraceUia(this, ApiCall::Clone, &apiMsg);*/

    return S_OK;
}

void UiaTextRange::_TranslatePointToScreen(til::point& clientPoint) const
{
    ClientToScreen(_getWindowHandle(), clientPoint.as_win32_point());
}

void UiaTextRange::_TranslatePointFromScreen(til::point& screenPoint) const
{
    ScreenToClient(_getWindowHandle(), screenPoint.as_win32_point());
}

HWND UiaTextRange::_getWindowHandle() const
{
    const auto provider = static_cast<ScreenInfoUiaProvider*>(_pProvider);
    return provider->GetWindowHandle();
}
Repository: microsoft/terminal
File: src/cascadia/TerminalCore/TerminalApi.cpp (C++)

Code before:
    if (!_suppressApplicationTitle)
    {
        _title.emplace(title);
        _pfnTitleChanged(_title.value());
    }
}

CursorType Terminal::GetUserDefaultCursorStyle() const noexcept
{
    _assertLocked();
    return _defaultCursorShape;
}

bool Terminal::ResizeWindow(const til::CoordType width, const til::CoordType height) noexcept
{
    // TODO: This will be needed to support various resizing sequences. See also GH#1860.
    _assertLocked();

    if (width <= 0 || height <= 0 || width > SHRT_MAX || height > SHRT_MAX)
    {
        return false;
    }

    if (_pfnWindowSizeChanged)
    {
        _pfnWindowSizeChanged(width, height);
        return true;
    }

    return false;
}

void Terminal::SetConsoleOutputCP(const unsigned int /*codepage*/) noexcept
{
    // TODO: This will be needed to support 8-bit charsets and DOCS sequences.
}

unsigned int Terminal::GetConsoleOutputCP() const noexcept
{
    // TODO: See SetConsoleOutputCP above.
    return CP_UTF8;
}

void Terminal::CopyToClipboard(wil::zwstring_view content)
{
    _pfnCopyToClipboard(content);
}

// Method Description:
// - Updates the taskbar progress indicator
// Arguments:

Review comment (suggestion):
Looks like the "audit mode" CI caught this:

```
src\cascadia\TerminalCore\TerminalApi.cpp(112,0): Error C26447: The function is declared 'noexcept' but calls function 'operator()()' which may throw exceptions (f.6).
```

I bet you can just remove `noexcept` from `Terminal::ResizeWindow`. or wrap it in a `try{} CATCH_LOG()`

Code after:
    if (!_suppressApplicationTitle)
    {
        _title.emplace(title);
        _pfnTitleChanged(_title.value());
    }
}

CursorType Terminal::GetUserDefaultCursorStyle() const noexcept
{
    _assertLocked();
    return _defaultCursorShape;
}

bool Terminal::ResizeWindow(const til::CoordType width, const til::CoordType height)
{
    // TODO: This will be needed to support various resizing sequences. See also GH#1860.
    _assertLocked();

    if (width <= 0 || height <= 0 || width > SHRT_MAX || height > SHRT_MAX)
    {
        return false;
    }

    if (_pfnWindowSizeChanged)
    {
        _pfnWindowSizeChanged(width, height);
        return true;
    }

    return false;
}

void Terminal::SetConsoleOutputCP(const unsigned int /*codepage*/) noexcept
{
    // TODO: This will be needed to support 8-bit charsets and DOCS sequences.
}

unsigned int Terminal::GetConsoleOutputCP() const noexcept
{
    // TODO: See SetConsoleOutputCP above.
    return CP_UTF8;
}

void Terminal::CopyToClipboard(wil::zwstring_view content)
{
    _pfnCopyToClipboard(content);
}

// Method Description:
// - Updates the taskbar progress indicator
// Arguments:
Repository: microsoft/terminal
File: src/buffer/out/OutputCell.cpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"

#include "OutputCell.hpp"

#include "../../types/inc/GlyphWidth.hpp"
#include "../../types/inc/convert.hpp"
#include "../../inc/conattrs.hpp"

// Misdiagnosis in MSVC 17.11: Referencing global constants in the member initializer
// list leads to this warning. Can probably be removed in the future.
#pragma warning(disable : 26493) // Don't use C-style casts (type.4).)

static constexpr TextAttribute InvalidTextAttribute{ INVALID_COLOR, INVALID_COLOR };

OutputCell::OutputCell() noexcept :
    _text{},
    _dbcsAttribute{},
    _textAttribute{ InvalidTextAttribute },
    _behavior{ TextAttributeBehavior::Stored }
{
}

OutputCell::OutputCell(const std::wstring_view charData,
                       const DbcsAttribute dbcsAttribute,
                       const TextAttributeBehavior behavior) :
    _text{ UNICODE_INVALID },
    _dbcsAttribute{ dbcsAttribute },
    _textAttribute{ InvalidTextAttribute },
    _behavior{ behavior }
{
    THROW_HR_IF(E_INVALIDARG, charData.empty());
    _setFromStringView(charData);
    _setFromBehavior(behavior);
}

Review comment (suggestion):
mark these with `BODGY` since that's how we mark things where we had to work around dumb platform problems

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"

#include "OutputCell.hpp"

#include "../../types/inc/GlyphWidth.hpp"
#include "../../types/inc/convert.hpp"
#include "../../inc/conattrs.hpp"

// BODGY: Misdiagnosis in MSVC 17.11: Referencing global constants in the member
// initializer list leads to this warning. Can probably be removed in the future.
#pragma warning(disable : 26493) // Don't use C-style casts (type.4).)

static constexpr TextAttribute InvalidTextAttribute{ INVALID_COLOR, INVALID_COLOR };

OutputCell::OutputCell() noexcept :
    _text{},
    _dbcsAttribute{},
    _textAttribute{ InvalidTextAttribute },
    _behavior{ TextAttributeBehavior::Stored }
{
}

OutputCell::OutputCell(const std::wstring_view charData,
                       const DbcsAttribute dbcsAttribute,
                       const TextAttributeBehavior behavior) :
    _text{ UNICODE_INVALID },
    _dbcsAttribute{ dbcsAttribute },
    _textAttribute{ InvalidTextAttribute },
    _behavior{ behavior }
{
    THROW_HR_IF(E_INVALIDARG, charData.empty());
    _setFromStringView(charData);
    _setFromBehavior(behavior);
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp (C++)

Code before:

                values[index] = curVal;
            }
        }
        catch (...)
        {
            values.fill(0);
            LOG_CAUGHT_EXCEPTION();
        }

        const auto result = fmt::format(FMT_COMPILE(L"{:.6f}"), fmt::join(values, L","));

        return winrt::hstring{ result };
    }

    double ProfileViewModel::_GetPaddingValue(PaddingDirection paddingDirection) const
    {
        std::wstring_view remaining{ Padding() };
        uint32_t paddingIndex = static_cast<uint32_t>(paddingDirection);
        double paddingValue = 0.;

        try
        {
            for (uint32_t index = 0; !remaining.empty(); ++index)
            {
                const std::wstring token{ til::prefix_split(remaining, L',') };
                auto curVal = std::stod(token);

                if (paddingIndex == index)
                {
                    paddingValue = curVal;
                    break;
                }
            }
        }
        catch (...)
        {
            paddingValue = 0.;
            LOG_CAUGHT_EXCEPTION();
        }

        return paddingValue;
    }

    Model::TerminalSettings ProfileViewModel::TermSettings() const
    {
        return Model::TerminalSettings::CreateForPreview(_appSettings, _profile);
    }

    // Method Description:
    // - Updates the lists of fonts and sorts them alphabetically

Review comment (suggestion):
My only concern is that this pair of functions will mis-handle 1-part and 2-part paddings:

padding `8` should be parsed as `8,8,8,8`; padding `8,4` should be parsed as `8,4,8,4`

everything else, I'm totally ready to sign off!

Code after:

                if (paddingIndex == index)
                {
                    curVal = newPaddingValue;
                }

                values[index++] = curVal;

                if (index >= values.size())
                {
                    break;
                }
            }
        }
        catch (...)
        {
            values.fill(0);
            LOG_CAUGHT_EXCEPTION();
        }

        const auto result = fmt::format(FMT_COMPILE(L"{:.6f}"), fmt::join(values, L","));

        return winrt::hstring{ result };
    }

    double ProfileViewModel::_GetPaddingValue(PaddingDirection paddingDirection) const
    {
        std::wstring_view padding{ Padding() };
        uint32_t paddingIndex = static_cast<uint32_t>(paddingDirection);
        std::array<double, 4> paddingValues{};
        double paddingValue = 0.;
        uint32_t index = 0;

        try
        {
            for (const auto& token : til::split_iterator{ padding, L',' })
            {
                auto curVal = std::stod(std::wstring{ token });

                paddingValues[index++] = curVal;

                if (index >= paddingValues.size())
                {
                    break;
                }
            }
        }
        catch (...)
        {
            paddingValue = 0.;
            LOG_CAUGHT_EXCEPTION();
Repository: microsoft/terminal
File: src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp (C++)

Code before:
    if (_fullscreen)
    {
        _UpdateTitlebarVisibility();
    }
}

void NonClientIslandWindow::_UpdateTitlebarVisibility()
{
    if (!_titlebar)
    {
        return;
    }

    const auto showTitlebar = _IsTitlebarVisible();
    _titlebar.Visibility(showTitlebar ? Visibility::Visible : Visibility::Collapsed);
    _titlebar.FullscreenChanged(_fullscreen);
}

// Method Description:
// - Returns true if the titlebar is visible. For borderless mode (aka "focus mode"),
//   this will return false. For fullscreen, this will return false unless the user
//   has enabled fullscreen tabs.
// Arguments:
// - <none>
// Return Value:
// - true if the titlebar is visible
bool NonClientIslandWindow::_IsTitlebarVisible() const
{
    return !_borderless && (!_fullscreen || _showTabsFullscreen);
}

void NonClientIslandWindow::SetTitlebarBackground(winrt::Windows::UI::Xaml::Media::Brush brush)
{
    _titlebar.Background(brush);
}

void NonClientIslandWindow::UseMica(const bool newValue, const double titlebarOpacity)
{
    // Stash internally if we're using Mica. If we aren't, we don't want to
    // totally blow away our titlebar with DwmExtendFrameIntoClientArea,
    // especially on Windows 10
    _useMica = newValue;
    _titlebarOpacity = titlebarOpacity;

    IslandWindow::UseMica(newValue, titlebarOpacity);

    _UpdateFrameMargins();
}


Review comment (suggestion):
```suggestion
// - true iff the titlebar is visible
```

(iff -> if and only if; not a typo!)

Code after:
    if (!_titlebar)
    {
        return;
    }

    const auto showTitlebar = _IsTitlebarVisible();
    _titlebar.Visibility(showTitlebar ? Visibility::Visible : Visibility::Collapsed);
    _titlebar.FullscreenChanged(_fullscreen);
}

// Method Description:
// - Returns true if the titlebar is visible. For borderless mode (aka "focus mode"),
//   this will return false. For fullscreen, this will return false unless the user
//   has enabled fullscreen tabs.
// Arguments:
// - <none>
// Return Value:
// - true iff the titlebar is visible
bool NonClientIslandWindow::_IsTitlebarVisible() const
{
    return !_borderless && (!_fullscreen || _showTabsFullscreen);
}

void NonClientIslandWindow::SetTitlebarBackground(winrt::Windows::UI::Xaml::Media::Brush brush)
{
    _titlebar.Background(brush);
}

void NonClientIslandWindow::UseMica(const bool newValue, const double titlebarOpacity)
{
    // Stash internally if we're using Mica. If we aren't, we don't want to
    // totally blow away our titlebar with DwmExtendFrameIntoClientArea,
    // especially on Windows 10
    _useMica = newValue;
    _titlebarOpacity = titlebarOpacity;

    IslandWindow::UseMica(newValue, titlebarOpacity);

    _UpdateFrameMargins();
}
Repository: microsoft/terminal
File: src/inc/til/winrt.h (C/C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

namespace til // Terminal Implementation Library. Also: "Today I Learned"
{
    template<typename T>
    struct property
    {
        explicit constexpr property(auto&&... args) noexcept :
            _value{ std::forward<decltype(args)>(args)... } {}

        property& operator=(const property& other) = default;

        T operator()() const noexcept(noexcept(static_cast<T>(_value)))
        {
            return _value;
        }
        void operator()(auto&& arg)
        {
            _value = std::forward<decltype(arg)>(arg);
        }
        explicit operator bool() const noexcept
        {
#ifdef WINRT_Windows_Foundation_H
            if constexpr (std::is_same_v<T, winrt::hstring>)
            {
                return !_value.empty();
            }
            else
#endif
            {
                return _value;
            }
        }

Review comment (suggestion):
This should be something like `noexcept(std::is_nothrow_constructible_v<T, decltype(args)...>`, aka:
𓀀 𓁐 𓁛 𓁼 𓄿 𓆄 𓆑 𓆟 𓆣 𓆭 𓈝 𓊝 𓊩 𓊯𓋑 𓌪 𓌳 𓍯 𓎵

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

namespace til // Terminal Implementation Library. Also: "Today I Learned"
{
    template<typename T>
    struct property
    {
        explicit constexpr property(auto&&... args) noexcept(std::is_nothrow_constructible_v<T, decltype(args)...>) :
            _value{ std::forward<decltype(args)>(args)... } {}

        property& operator=(const property& other) = default;

        T operator()() const noexcept(std::is_nothrow_copy_constructible<T>::value)
        {
            return _value;
        }
        void operator()(auto&& arg)
        {
            _value = std::forward<decltype(arg)>(arg);
        }
        explicit operator bool() const noexcept
        {
#ifdef WINRT_Windows_Foundation_H
            if constexpr (std::is_same_v<T, winrt::hstring>)
            {
                return !_value.empty();
            }
            else
#endif
            {
                return _value;
            }
        }
Repository: microsoft/terminal
File: src/inc/til/winrt.h (C/C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

namespace til // Terminal Implementation Library. Also: "Today I Learned"
{
    template<typename T>
    struct property
    {
        explicit constexpr property(auto&&... args) noexcept :
            _value{ std::forward<decltype(args)>(args)... } {}

        property& operator=(const property& other) = default;

        T operator()() const noexcept(noexcept(static_cast<T>(_value)))
        {
            return _value;
        }
        void operator()(auto&& arg)
        {
            _value = std::forward<decltype(arg)>(arg);
        }
        explicit operator bool() const noexcept
        {
#ifdef WINRT_Windows_Foundation_H
            if constexpr (std::is_same_v<T, winrt::hstring>)
            {
                return !_value.empty();
            }
            else
#endif
            {
                return _value;
            }
        }
        bool operator==(const property& other) const noexcept
        {
            return _value == other._value;
        }
        bool operator!=(const property& other) const noexcept

Review comment (suggestion):
I believe this should be `std::is_nothrow_copy_constructible`, but I think it would be even more ideal for us to just return `const T&` here and leave the unconditional `noexcept`.

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

namespace til // Terminal Implementation Library. Also: "Today I Learned"
{
    template<typename T>
    struct property
    {
        explicit constexpr property(auto&&... args) noexcept(std::is_nothrow_constructible_v<T, decltype(args)...>) :
            _value{ std::forward<decltype(args)>(args)... } {}

        property& operator=(const property& other) = default;

        T operator()() const noexcept(std::is_nothrow_copy_constructible<T>::value)
        {
            return _value;
        }
        void operator()(auto&& arg)
        {
            _value = std::forward<decltype(arg)>(arg);
        }
        explicit operator bool() const noexcept
        {
#ifdef WINRT_Windows_Foundation_H
            if constexpr (std::is_same_v<T, winrt::hstring>)
            {
                return !_value.empty();
            }
            else
#endif
            {
                return _value;
            }
        }
        bool operator==(const property& other) const noexcept
        {
            return _value == other._value;
        }
        bool operator!=(const property& other) const noexcept