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

Code before:
            return til::u16u8(val);
        }

        std::string TypeDescription() const
        {
            return "string";
        }
    };

    template<typename T>
    struct ConversionTrait<std::vector<T>>
    {
        std::vector<T> FromJson(const Json::Value& json)
        {
            std::vector<T> val;
            val.reserve(json.size());

            ConversionTrait<T> trait;
            for (const auto& element : json)
            {
                val.push_back(trait.FromJson(element));
            }

            return val;
        }

        bool CanConvert(const Json::Value& json) const
        {
            ConversionTrait<T> trait;
            return json.isArray() && std::all_of(json.begin(), json.end(), [trait](const auto& json) mutable -> bool { return trait.CanConvert(json); });
        }

        Json::Value ToJson(const std::vector<T>& val)
        {
            Json::Value json{ Json::arrayValue };

            ConversionTrait<T> trait;
            for (const auto& v : val)
            {
                json.append(trait.ToJson(v));
            }

            return json;
        }
        std::string TypeDescription() const
        {
            return fmt::format("{}[]", ConversionTrait<T>{}.TypeDescription());
        }
    };

    template<typename T>

Review comment (suggestion):
note to self: this is the place I should just check for null and skip

Code after:
            return til::u16u8(val);
        }

        std::string TypeDescription() const
        {
            return "string";
        }
    };

    template<typename T>
    struct ConversionTrait<std::vector<T>>
    {
        std::vector<T> FromJson(const Json::Value& json)
        {
            std::vector<T> val;
            val.reserve(json.size());

            ConversionTrait<T> trait;
            if (json.isArray())
            {
                for (const auto& element : json)
                {
                    val.push_back(trait.FromJson(element));
                }
            }
            else
            {
                val.push_back(trait.FromJson(json));
            }

            return val;
        }

        bool CanConvert(const Json::Value& json) const
        {
            ConversionTrait<T> trait;
            // If there's only one element provided, then see if we can convert
            // that single element into a length-1 array
            return (json.isArray() && std::all_of(json.begin(), json.end(), [trait](const auto& json) mutable -> bool { return trait.CanConvert(json); })) || trait.CanConvert(json);
        }

        Json::Value ToJson(const std::vector<T>& val)
        {
            Json::Value json{ Json::arrayValue };

            ConversionTrait<T> trait;
            for (const auto& v : val)
            {
                json.append(trait.ToJson(v));
            }
Repository: microsoft/terminal
File: src/renderer/base/fontinfo.cpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"

#include "../inc/FontInfo.hpp"

FontInfo::FontInfo(const std::wstring_view& faceName, const unsigned char family, const unsigned int weight, const COORD coordSize, const unsigned int codePage, const bool fSetDefaultRasterFont) noexcept :
    FontInfoBase(faceName, family, weight, fSetDefaultRasterFont, codePage),
    _coordSize(coordSize),
    _coordSizeUnscaled(coordSize),
    _didFallback(false)
{
    ValidateFont();
}

bool FontInfo::operator==(const FontInfo& other) noexcept
{
    return FontInfoBase::operator==(other) &&
           _coordSize == other._coordSize &&
           _coordSizeUnscaled == other._coordSizeUnscaled;
}

COORD FontInfo::GetUnscaledSize() const noexcept
{
    return _coordSizeUnscaled;
}

COORD FontInfo::GetSize() const noexcept
{
    return _coordSize;
}


Review comment (suggestion):
BTW The reason I scram these into a single line is because
* (They still fit on most monitors and...)
* Because it makes copy-pasting definitions into declarations and vice versa way easier IMHO

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"

#include "../inc/FontInfo.hpp"

FontInfo::FontInfo(const std::wstring_view& faceName,
                   const unsigned char family,
                   const unsigned int weight,
                   const COORD coordSize,
                   const unsigned int codePage,
                   const bool fSetDefaultRasterFont /* = false */) noexcept :
    FontInfoBase(faceName, family, weight, fSetDefaultRasterFont, codePage),
    _coordSize(coordSize),
    _coordSizeUnscaled(coordSize),
    _didFallback(false)
{
    ValidateFont();
}

bool FontInfo::operator==(const FontInfo& other) noexcept
{
    return FontInfoBase::operator==(other) &&
           _coordSize == other._coordSize &&
           _coordSizeUnscaled == other._coordSizeUnscaled;
}

COORD FontInfo::GetUnscaledSize() const noexcept
{
    return _coordSizeUnscaled;
}
Repository: microsoft/terminal
File: src/cascadia/TerminalApp/TabRowControl.h (C/C++)

Code before:
// Licensed under the MIT license.

#pragma once

#include "winrt/Microsoft.UI.Xaml.Controls.h"
#include "../../cascadia/inc/cppwinrt_utils.h"

#include "TabRowControl.g.h"

namespace winrt::TerminalApp::implementation
{
    struct TabRowControl : TabRowControlT<TabRowControl>
    {
        TabRowControl();

        void OnNewTabButtonClick(Windows::Foundation::IInspectable const& sender, Microsoft::UI::Xaml::Controls::SplitButtonClickEventArgs const& args);
        void OnNewTabButtonDrop(winrt::Windows::Foundation::IInspectable const& sender, winrt::Windows::UI::Xaml::DragEventArgs const& e);
        void OnNewTabButtonDragOver(winrt::Windows::Foundation::IInspectable const& sender, winrt::Windows::UI::Xaml::DragEventArgs const& e);

        WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
        WINRT_OBSERVABLE_PROPERTY(bool, ShowElevationShield, _PropertyChangedHandlers, false);
    };
}

namespace winrt::TerminalApp::factory_implementation
{
    struct TabRowControl : TabRowControlT<TabRowControl, implementation::TabRowControl>
    {
    };
}


Review comment (suggestion):
Outside the scope of this... but maybe we should prefix these macros with something (like how the WIL ones do WI_FLAG_SET) so we understand which ones are a part of our magic and which ones are a part of the actual cppwinrt packages.

Code after:
// Licensed under the MIT license.

#pragma once

#include "winrt/Microsoft.UI.Xaml.Controls.h"

#include "TabRowControl.g.h"

namespace winrt::TerminalApp::implementation
{
    struct TabRowControl : TabRowControlT<TabRowControl>
    {
        TabRowControl();

        void OnNewTabButtonClick(Windows::Foundation::IInspectable const& sender, Microsoft::UI::Xaml::Controls::SplitButtonClickEventArgs const& args);
        void OnNewTabButtonDrop(winrt::Windows::Foundation::IInspectable const& sender, winrt::Windows::UI::Xaml::DragEventArgs const& e);
        void OnNewTabButtonDragOver(winrt::Windows::Foundation::IInspectable const& sender, winrt::Windows::UI::Xaml::DragEventArgs const& e);

        WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
        WINRT_OBSERVABLE_PROPERTY(bool, ShowElevationShield, _PropertyChangedHandlers, false);
    };
}

namespace winrt::TerminalApp::factory_implementation
{
    BASIC_FACTORY(TabRowControl);
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsModel/HashUtils.h (C/C++)

Code before:
--*/

#pragma once

namespace Microsoft::Terminal::Settings::Model::HashUtils
{
    // This is a helper template function for hashing multiple variables in conjunction to each other.
    template<typename T>
    constexpr size_t HashProperty(const T& val)
    {
        std::hash<T> hashFunc;
        return hashFunc(val);
    }

    template<typename T, typename... Args>
    constexpr size_t HashProperty(const T& val, Args&&... more)
    {
        // Inspired by boost::hash_combine, which causes this effect...
        //   seed ^= hash_value(v) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
        // Source: https://www.boost.org/doc/libs/1_35_0/doc/html/boost/hash_combine_id241013.html
        const auto seed{ HashProperty(val) };
        return seed ^ (0x9e3779b9 + (seed << 6) + (seed >> 2) + HashProperty(std::forward<Args>(more)...));
    }

    template<>
    constexpr size_t HashProperty(const til::color& val)
    {
        return HashProperty(val.a, val.r, val.g, val.b);
    }

#ifdef WINRT_Windows_Foundation_H
    template<typename T>
    constexpr size_t HashProperty(const winrt::Windows::Foundation::IReference<T>& val)
    {
        return val ? HashProperty(val.Value()) : 0;
    }
#endif

#ifdef WINRT_Windows_UI_H
    template<>
    constexpr size_t HashProperty(const winrt::Windows::UI::Color& val)
    {
        return HashProperty(til::color{ val });
    }
#endif

#ifdef WINRT_Microsoft_Terminal_Core_H
    template<>
    constexpr size_t HashProperty(const winrt::Microsoft::Terminal::Core::Color& val)
    {
        return HashProperty(til::color{ val });

Review comment (suggestion):
wait a minute, wat? this hashes the pointer to the `NewTerminalArgs`, doesn't it? That should never compare-same...

Code after:
--*/

#pragma once

#include <til/hash.h>

namespace til
{
    template<typename T>
    struct hash_trait<winrt::Windows::Foundation::IReference<T>>
    {
        constexpr void operator()(hasher& h, const winrt::Windows::Foundation::IReference<T>& v) const noexcept
        {
            if (v)
            {
                h.write(v.Value());
            }
        }
    };

    template<>
    struct hash_trait<winrt::Microsoft::Terminal::Settings::Model::NewTerminalArgs>
    {
        void operator()(hasher& h, const winrt::Microsoft::Terminal::Settings::Model::NewTerminalArgs& value) const noexcept
        {
            h.write(winrt::get_abi(value));
        }
    };

    template<>
    struct hash_trait<winrt::hstring>
    {
        void operator()(hasher& h, const winrt::hstring& value) const noexcept
        {
            h.write(reinterpret_cast<const uint8_t*>(value.data()), value.size() * sizeof(wchar_t));
        }
    };
}
Repository: microsoft/terminal
File: src/types/inc/sgrStack.hpp (C++)

Code before:
/*++
Copyright (c) Microsoft Corporation
Licensed under the MIT license.

Module Name:
- sgrStack.hpp

Abstract:
- Encapsulates logic for the XTPUSHSGR / XTPOPSGR VT control sequences, which save and
  restore text attributes on a stack.

--*/

#pragma once

#include "..\..\buffer\out\TextAttribute.hpp"
#include "..\..\inc\til\enumset.h"
#include "..\..\terminal\adapter\DispatchTypes.hpp"

namespace Microsoft::Console::VirtualTerminal
{
    class SgrStack
    {
    public:
        SgrStack() noexcept;

        // Method Description:
        // - Saves the specified text attributes onto an internal stack.
        // Arguments:
        // - currentAttributes - The attributes to save onto the stack.
        // - options - If none supplied, the full attributes are saved. Else only the
        //   specified parts of currentAttributes are saved.
        // Return Value:
        // - <none>
        void Push(const TextAttribute& currentAttributes,
                  const VTParameters options) noexcept;

        // Method Description:
        // - Restores text attributes by removing from the top of the internal stack,
        //   combining them with the supplied currentAttributes, if appropriate.
        // Arguments:
        // - currentAttributes - The current text attributes. If only a portion of

Review comment (suggestion):
Oh and I think this should be `#include <til/enumset.h>` for consistency...

Code after:
/*++
Copyright (c) Microsoft Corporation
Licensed under the MIT license.

Module Name:
- sgrStack.hpp

Abstract:
- Encapsulates logic for the XTPUSHSGR / XTPOPSGR VT control sequences, which save and
  restore text attributes on a stack.

--*/

#pragma once

#include "../../buffer/out/TextAttribute.hpp"
#include "../../terminal/adapter/DispatchTypes.hpp"
#include <til/enumset.h>

namespace Microsoft::Console::VirtualTerminal
{
    class SgrStack
    {
    public:
        SgrStack() noexcept;

        // Method Description:
        // - Saves the specified text attributes onto an internal stack.
        // Arguments:
        // - currentAttributes - The attributes to save onto the stack.
        // - options - If none supplied, the full attributes are saved. Else only the
        //   specified parts of currentAttributes are saved.
        // Return Value:
        // - <none>
        void Push(const TextAttribute& currentAttributes,
                  const VTParameters options) noexcept;

        // Method Description:
        // - Restores text attributes by removing from the top of the internal stack,
        //   combining them with the supplied currentAttributes, if appropriate.
        // Arguments:
        // - currentAttributes - The current text attributes. If only a portion of
Repository: microsoft/terminal
File: src/host/consoleInformation.cpp (C++)

Code before:
    CtrlFlags(0),
    LimitingProcessId(0),
    // ColorTable initialized below
    // CPInfo initialized below
    // OutputCPInfo initialized below
    _cookedReadData(nullptr),
    ConsoleIme{},
    _vtIo(),
    _blinker{},
    renderData{}
{
    ZeroMemory((void*)&CPInfo, sizeof(CPInfo));
    ZeroMemory((void*)&OutputCPInfo, sizeof(OutputCPInfo));
}

static thread_local ULONG recursionCount = 0;
static til::ticket_lock lock;

bool CONSOLE_INFORMATION::IsConsoleLocked()
{
    return recursionCount != 0;
}

#pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.")
void CONSOLE_INFORMATION::LockConsole()
{
    const auto rc = ++recursionCount;
    FAIL_FAST_IF(rc == 0);
    if (rc == 1)
    {
        lock.lock();
    }
}

#pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.")
void CONSOLE_INFORMATION::UnlockConsole()
{
    const auto rc = --recursionCount;
    FAIL_FAST_IF(rc == ULONG_MAX);
    if (rc == 0)
    {
        lock.unlock();
    }
}

ULONG CONSOLE_INFORMATION::GetCSRecursionCount()
{
    return recursionCount;
}

// Routine Description:

Review comment (suggestion):
Ah btw: You can't have thread locals as class members. They only work as statics / in the global scope.

Code after:
    CtrlFlags(0),
    LimitingProcessId(0),
    // ColorTable initialized below
    // CPInfo initialized below
    // OutputCPInfo initialized below
    _cookedReadData(nullptr),
    ConsoleIme{},
    _vtIo(),
    _blinker{},
    renderData{}
{
    ZeroMemory((void*)&CPInfo, sizeof(CPInfo));
    ZeroMemory((void*)&OutputCPInfo, sizeof(OutputCPInfo));
}

// Access to thread-local variables is thread-safe, because each thread
// gets its own copy of this variable with a default value of 0.
//
// Whenever we want to acquire the lock we increment recursionCount and on
// each release we decrement it again. We can then make the lock safe for
// reentrancy by only acquiring/releasing the lock if the recursionCount is 0.
// In a sense, recursionCount is counting the actual function
// call recursion depth of the caller. This works as long as
// the caller makes sure to properly call Unlock() once for each Lock().
static thread_local ULONG recursionCount = 0;
static til::ticket_lock lock;

bool CONSOLE_INFORMATION::IsConsoleLocked()
{
    return recursionCount != 0;
}

#pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.")
void CONSOLE_INFORMATION::LockConsole()
{
    // See description of recursionCount a few lines above.
    const auto rc = ++recursionCount;
    FAIL_FAST_IF(rc == 0);
    if (rc == 1)
    {
        lock.lock();
    }
}

#pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.")
void CONSOLE_INFORMATION::UnlockConsole()
{
    // See description of recursionCount a few lines above.
    const auto rc = --recursionCount;
    FAIL_FAST_IF(rc == ULONG_MAX);
    if (rc == 0)
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/AddProfile.cpp (C++)

Code before:
    }

    void AddProfile::OnNavigatedTo(const NavigationEventArgs& e)
    {
        _State = e.Parameter().as<Editor::AddProfilePageNavigationState>();
    }

    void AddProfile::AddNewClick(const IInspectable& /*sender*/,
                                 const Windows::UI::Xaml::RoutedEventArgs& /*eventArgs*/)
    {
        _State.RequestAddNew();
    }

    void AddProfile::DuplicateClick(const IInspectable& /*sender*/,
                                    const Windows::UI::Xaml::RoutedEventArgs& /*eventArgs*/)
    {
        if (const auto selected = Profiles().SelectedItem())
        {
            _State.RequestDuplicate(selected.try_as<Model::Profile>().Guid());
        }
    }

    void AddProfile::ProfilesSelectionChanged(const IInspectable& /*sender*/,
                                              const Windows::UI::Xaml::RoutedEventArgs& /*eventArgs*/)
    {
        if (!IsProfileSelected())
        {
            IsProfileSelected(true);
        }
    }
}


Review comment (suggestion):
```suggestion
        if (!_IsProfileSelected)
```
nit: I think this'll be faster since we're not calling the function, technically

Code after:
    }

    void AddProfile::OnNavigatedTo(const NavigationEventArgs& e)
    {
        _State = e.Parameter().as<Editor::AddProfilePageNavigationState>();
    }

    void AddProfile::AddNewClick(const IInspectable& /*sender*/,
                                 const Windows::UI::Xaml::RoutedEventArgs& /*eventArgs*/)
    {
        _State.RequestAddNew();
    }

    void AddProfile::DuplicateClick(const IInspectable& /*sender*/,
                                    const Windows::UI::Xaml::RoutedEventArgs& /*eventArgs*/)
    {
        if (const auto selected = Profiles().SelectedItem())
        {
            _State.RequestDuplicate(selected.try_as<Model::Profile>().Guid());
        }
    }

    void AddProfile::ProfilesSelectionChanged(const IInspectable& /*sender*/,
                                              const Windows::UI::Xaml::RoutedEventArgs& /*eventArgs*/)
    {
        if (!_IsProfileSelected)
        {
            IsProfileSelected(true);
        }
    }
}
Repository: microsoft/terminal
File: src/host/output.cpp (C++)

Code before:

    gci.ConsoleIme.RefreshAreaAttributes();

    // Write data to screen.
    WriteToScreen(screenInfo, screenInfo.GetViewport());
}

// TODO: MSFT 9450717 This should join the ProcessList class when CtrlEvents become moved into the server. https://osgvsowi/9450717
void CloseConsoleProcessState()
{
    const CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
    // If there are no connected processes, sending control events is pointless as there's no one do send them to. In
    // this case we'll just exit conhost.

    // N.B. We can get into this state when a process has a reference to the console but hasn't connected. For example,
    //      when it's created suspended and never resumed.
    if (gci.ProcessHandleList.IsEmpty())
    {
        ServiceLocator::RundownAndExit(STATUS_SUCCESS);
    }

    HandleCtrlEvent(CTRL_CLOSE_EVENT);

    // MSFT:19419231
    // Ensure ctrl events are processed before exiting,
    // no matter whether the console is locked or not.
    LockConsole();
    ProcessCtrlEvents();
}


Review comment (suggestion):
May want to add an archeological 
```suggestion
    // no matter whether the console is locked or not.
    // GH#12181: we used to "jiggle the handle" here to trigger ProcessCtrlEvents in the scenario where we were called outside of lock (e.g. PtySignalInputThread::_GetData). WE no longer need to worry about that.....
```

Code after:

    gci.ConsoleIme.RefreshAreaAttributes();

    // Write data to screen.
    WriteToScreen(screenInfo, screenInfo.GetViewport());
}

// TODO: MSFT 9450717 This should join the ProcessList class when CtrlEvents become moved into the server. https://osgvsowi/9450717
void CloseConsoleProcessState()
{
    const CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
    // If there are no connected processes, sending control events is pointless as there's no one do send them to. In
    // this case we'll just exit conhost.

    // N.B. We can get into this state when a process has a reference to the console but hasn't connected. For example,
    //      when it's created suspended and never resumed.
    if (gci.ProcessHandleList.IsEmpty())
    {
        ServiceLocator::RundownAndExit(STATUS_SUCCESS);
    }

    HandleCtrlEvent(CTRL_CLOSE_EVENT);

    // Jiggle the handle: (see MSFT:19419231)
    // When we call this function, we'll only actually close the console once
    //      we're totally unlocked. If our caller has the console locked, great,
    //      we'll dispatch the ctrl event once they unlock. However, if they're
    //      not running under lock (eg PtySignalInputThread::_GetData), then the
    //      ctrl event will never actually get dispatched.
    // So, lock and unlock here, to make sure the ctrl event gets handled.
    LockConsole();
    UnlockConsole();
}