ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/terminal
File: src/renderer/dx/DxFontRenderData.cpp (C++)

Code before:
}

// Routine Description:
// - Digs through the directory that the current executable is running within to find
//   any TTF files sitting next to it.
// Arguments:
// - <none>
// Return Value:
// - Iterable collection of filesystem paths, one per font file that was found
[[nodiscard]] std::vector<std::filesystem::path> DxFontRenderData::s_GetNearbyFonts()
{
    std::vector<std::filesystem::path> paths;

    // Find the directory we're running from then enumerate all the TTF files
    // sitting next to us.
    const std::filesystem::path module{ wil::GetModuleFileNameW<std::wstring>(nullptr) };
    const auto folder = module.parent_path();

    for (auto& p : std::filesystem::directory_iterator(folder))
    {
        if (p.is_regular_file())
        {
            auto extension = p.path().extension().wstring();
            std::transform(extension.begin(), extension.end(), extension.begin(), std::towlower);

            const std::wstring_view ttfExtension{ L".ttf" };
            if (ttfExtension == extension)
            {
                paths.push_back(p);
            }
        }
    }

    return paths;
}


Review comment (suggestion):
```suggestion
            static constexpr std::wstring_view ttfExtension{ L".ttf" };
```

Code after:
        {
            _userLocaleName = { localeName.data() };
        }
        else
        {
            _userLocaleName = { FALLBACK_LOCALE.data(), FALLBACK_LOCALE.size() };
        }
    }

    return _userLocaleName;
}

// Routine Description:
// - Digs through the directory that the current executable is running within to find
//   any TTF files sitting next to it.
// Arguments:
// - <none>
// Return Value:
// - Iterable collection of filesystem paths, one per font file that was found
[[nodiscard]] std::vector<std::filesystem::path> DxFontRenderData::s_GetNearbyFonts()
{
    std::vector<std::filesystem::path> paths;

    // Find the directory we're running from then enumerate all the TTF files
    // sitting next to us.
    const std::filesystem::path module{ wil::GetModuleFileNameW<std::wstring>(nullptr) };
    const auto folder{ module.parent_path() };

    for (auto& p : std::filesystem::directory_iterator(folder))
    {
        if (p.is_regular_file())
        {
            auto extension = p.path().extension().wstring();
            std::transform(extension.begin(), extension.end(), extension.begin(), std::towlower);

            static constexpr std::wstring_view ttfExtension{ L".ttf" };
            if (ttfExtension == extension)
            {
                paths.push_back(p);
            }
        }
    }

    return paths;
}
Repository: microsoft/terminal
File: src/host/ft_host/API_MultipleInflightMessageTests.cpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"

class MultipleInflightMessageTests
{
    BEGIN_TEST_CLASS(MultipleInflightMessageTests)
    END_TEST_CLASS()

    TEST_METHOD(WriteWhileReadingInputCrash)
    {
        BEGIN_TEST_METHOD_PROPERTIES()
            TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method") // Don't pollute other tests by isolating our potential crash and buffer resizing to this test.
        END_TEST_METHOD_PROPERTIES()

        using namespace std::string_view_literals;

        const auto inputHandle = GetStdInputHandle();
        const auto outputHandle = GetStdOutputHandle();

        DWORD originalConsoleMode{};
        VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleMode(inputHandle, &originalConsoleMode));
        auto restoreMode{ wil::scope_exit([=]() {
            SetConsoleMode(inputHandle, originalConsoleMode);
        }) };
        VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleMode(inputHandle, ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT));

        // Prime the console with some fake input records.
        const std::array<INPUT_RECORD, 4> inputRecords{
            INPUT_RECORD{ KEY_EVENT, KEY_EVENT_RECORD{ TRUE, 1, 'h', 0, L'h', 0 } },

Review comment (bug):
It might be nice to add a comment here, explaining that this test ensures that no regressions occurs, since this bug was fixed in `c0ab9cb5b55d745a1528c7429615c71e0745ed8d`.

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"

class MultipleInflightMessageTests
{
    BEGIN_TEST_CLASS(MultipleInflightMessageTests)
    END_TEST_CLASS()

    // This test is intended to make sure that we do not regress after the _handlePostCharInputLoop fix in OpenConsole:c0ab9cb5b
    TEST_METHOD(WriteWhileReadingInputCrash)
    {
        BEGIN_TEST_METHOD_PROPERTIES()
            TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method") // Don't pollute other tests by isolating our potential crash and buffer resizing to this test.
        END_TEST_METHOD_PROPERTIES()

        using namespace std::string_view_literals;

        const auto inputHandle = GetStdInputHandle();
        const auto outputHandle = GetStdOutputHandle();

        DWORD originalConsoleMode{};
        VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleMode(inputHandle, &originalConsoleMode));
        auto restoreMode{ wil::scope_exit([=]() {
            SetConsoleMode(inputHandle, originalConsoleMode);
        }) };
        VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleMode(inputHandle, ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT));

        // Prime the console with some fake input records.
        const std::array<INPUT_RECORD, 4> inputRecords{
Repository: microsoft/terminal
File: src/cascadia/WindowsTerminal/icon.cpp (C++)

Code before:
    }

    return iconResource;
}

HANDLE GetActiveAppIconHandle(int size)
{
    auto iconResource{ MAKEINTRESOURCEW(_GetActiveAppIconResource()) };

    auto smXIcon = size == ICON_SMALL ? SM_CXSMICON : SM_CXICON;
    auto smYIcon = size == ICON_SMALL ? SM_CYSMICON : SM_CYICON;

    // These handles are loaded with LR_SHARED, so they are safe to "leak".
    HANDLE hIcon{ LoadImageW(wil::GetModuleInstanceHandle(), iconResource, IMAGE_ICON, GetSystemMetrics(smXIcon), GetSystemMetrics(smYIcon), LR_SHARED) };
    LOG_LAST_ERROR_IF_NULL(hIcon);

    return hIcon;
}

void UpdateWindowIconForActiveMetrics(HWND window)
{
    if (auto smallIcon = GetActiveAppIconHandle(ICON_SMALL))
    {
        SendMessageW(window, WM_SETICON, ICON_SMALL, reinterpret_cast<LPARAM>(smallIcon));
    }
    else if (auto largeIcon = GetActiveAppIconHandle(ICON_BIG))
    {
        SendMessageW(window, WM_SETICON, ICON_BIG, reinterpret_cast<LPARAM>(largeIcon));
    }
}


Review comment (bug):
This one probably should not be an *else if* -- it will only trigger if we fail to load the small icon. We definitely want to do each of these independently!

Code after:
    }

    return iconResource;
}

HANDLE GetActiveAppIconHandle(int size)
{
    auto iconResource{ MAKEINTRESOURCEW(_GetActiveAppIconResource()) };

    const auto smXIcon = size == ICON_SMALL ? SM_CXSMICON : SM_CXICON;
    const auto smYIcon = size == ICON_SMALL ? SM_CYSMICON : SM_CYICON;

    // These handles are loaded with LR_SHARED, so they are safe to "leak".
    HANDLE hIcon{ LoadImageW(wil::GetModuleInstanceHandle(), iconResource, IMAGE_ICON, GetSystemMetrics(smXIcon), GetSystemMetrics(smYIcon), LR_SHARED) };
    LOG_LAST_ERROR_IF_NULL(hIcon);

    return hIcon;
}

void UpdateWindowIconForActiveMetrics(HWND window)
{
    if (auto smallIcon = GetActiveAppIconHandle(ICON_SMALL))
    {
        SendMessageW(window, WM_SETICON, ICON_SMALL, reinterpret_cast<LPARAM>(smallIcon));
    }
    if (auto largeIcon = GetActiveAppIconHandle(ICON_BIG))
    {
        SendMessageW(window, WM_SETICON, ICON_BIG, reinterpret_cast<LPARAM>(largeIcon));
    }
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/GlobalAppearance.cpp (C++)

Code before:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "pch.h"
#include "GlobalAppearance.h"
#include "GlobalAppearance.g.cpp"
#include "GlobalAppearancePageNavigationState.g.cpp"

#include <LibraryResources.h>
#include <numeric>

#include "EnumEntry.h"

using namespace winrt;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Navigation;
using namespace winrt::Windows::UI::Xaml::Controls;
using namespace winrt::Microsoft::Terminal::Settings::Model;
using namespace winrt::Windows::Foundation::Collections;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    // For ComboBox an empty SelectedItem string denotes no selection.
    // What we want instead is for "Use system language" to be selected by default.
    // --> "und" is synonymous for "Use system language".
    constexpr std::wstring_view systemLanguageTag{ L"und" };

    GlobalAppearance::GlobalAppearance()
    {
        InitializeComponent();

        INITIALIZE_BINDABLE_ENUM_SETTING(Theme, ElementTheme, winrt::Windows::UI::Xaml::ElementTheme, L"Globals_Theme", L"Content");
        INITIALIZE_BINDABLE_ENUM_SETTING(TabWidthMode, TabViewWidthMode, winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode, L"Globals_TabWidthMode", L"Content");
    }


Review comment (suggestion):
This should be part of the precompiled header precomp.h or pch.h and not be necessary here

`<numeric>` should be in LibraryIncludes

Code after:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "pch.h"
#include "EnumEntry.h"
#include "GlobalAppearance.h"
#include "GlobalAppearance.g.cpp"
#include "GlobalAppearancePageNavigationState.g.cpp"

#include <LibraryResources.h>

using namespace winrt;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Navigation;
using namespace winrt::Windows::UI::Xaml::Controls;
using namespace winrt::Microsoft::Terminal::Settings::Model;
using namespace winrt::Windows::Foundation::Collections;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    // For ComboBox an empty SelectedItem string denotes no selection.
    // What we want instead is for "Use system language" to be selected by default.
    // --> "und" is synonymous for "Use system language".
    constexpr std::wstring_view systemLanguageTag{ L"und" };

    GlobalAppearance::GlobalAppearance()
    {
        InitializeComponent();

        INITIALIZE_BINDABLE_ENUM_SETTING(Theme, ElementTheme, winrt::Windows::UI::Xaml::ElementTheme, L"Globals_Theme", L"Content");
        INITIALIZE_BINDABLE_ENUM_SETTING(TabWidthMode, TabViewWidthMode, winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode, L"Globals_TabWidthMode", L"Content");
    }

    void GlobalAppearance::OnNavigatedTo(const NavigationEventArgs& e)
    {
Repository: microsoft/terminal
File: src/cascadia/TerminalApp/App.base.h (C/C++)

Code before:
namespace winrt::TerminalApp::implementation
{
    template<typename D, typename... I>
    struct App_baseWithProvider : public App_base<D, ::winrt::Windows::UI::Xaml::Markup::IXamlMetadataProvider>
    {
        using IXamlType = ::winrt::Windows::UI::Xaml::Markup::IXamlType;

        IXamlType GetXamlType(::winrt::Windows::UI::Xaml::Interop::TypeName const& type)
        {
            return AppProvider()->GetXamlType(type);
        }

        IXamlType GetXamlType(::winrt::hstring const& fullName)
        {
            return AppProvider()->GetXamlType(fullName);
        }

        ::winrt::com_array<::winrt::Windows::UI::Xaml::Markup::XmlnsDefinition> GetXmlnsDefinitions()
        {
            return AppProvider()->GetXmlnsDefinitions();
        }

    private:
        bool _contentLoaded{ false };
        std::shared_ptr<winrt::TerminalApp::XamlMetaDataProvider> _appProvider;
        std::shared_ptr<winrt::TerminalApp::XamlMetaDataProvider> AppProvider()
        {
            if (!_appProvider)
            {
                _appProvider = std::make_shared<winrt::TerminalApp::XamlMetaDataProvider>();
            }
            return _appProvider;
        }
    };

    template<typename D, typename... I>
    using AppT2 = App_baseWithProvider<D, I...>;
}


Review comment (bug):
my only worry with this one is that... we're never supposed to use shared_ptr for winrt things.

We may want to just make this...

```
winrt::TerminalApp::XamlMetaDataProvider _appProvider{ nullptr };
```

and then just instantiate one down on line 32.

Code after:
namespace winrt::TerminalApp::implementation
{
    template<typename D, typename... I>
    struct App_baseWithProvider : public App_base<D, ::winrt::Windows::UI::Xaml::Markup::IXamlMetadataProvider>
    {
        using IXamlType = ::winrt::Windows::UI::Xaml::Markup::IXamlType;

        IXamlType GetXamlType(::winrt::Windows::UI::Xaml::Interop::TypeName const& type)
        {
            return _appProvider.GetXamlType(type);
        }

        IXamlType GetXamlType(::winrt::hstring const& fullName)
        {
            return _appProvider.GetXamlType(fullName);
        }

        ::winrt::com_array<::winrt::Windows::UI::Xaml::Markup::XmlnsDefinition> GetXmlnsDefinitions()
        {
            return _appProvider.GetXmlnsDefinitions();
        }

    private:
        bool _contentLoaded{ false };
        winrt::TerminalApp::XamlMetaDataProvider _appProvider;
    };

    template<typename D, typename... I>
    using AppT2 = App_baseWithProvider<D, I...>;
}
Repository: microsoft/terminal
File: src/cascadia/TerminalApp/App.base.h (C/C++)

Code before:
namespace winrt::TerminalApp::implementation
{
    template<typename D, typename... I>
    struct App_baseWithProvider : public App_base<D, ::winrt::Windows::UI::Xaml::Markup::IXamlMetadataProvider>
    {
        using IXamlType = ::winrt::Windows::UI::Xaml::Markup::IXamlType;

        IXamlType GetXamlType(::winrt::Windows::UI::Xaml::Interop::TypeName const& type)
        {
            return AppProvider().GetXamlType(type);
        }

        IXamlType GetXamlType(::winrt::hstring const& fullName)
        {
            return AppProvider().GetXamlType(fullName);
        }

        ::winrt::com_array<::winrt::Windows::UI::Xaml::Markup::XmlnsDefinition> GetXmlnsDefinitions()
        {
            return AppProvider().GetXmlnsDefinitions();
        }

    private:
        bool _contentLoaded{ false };
        winrt::TerminalApp::XamlMetaDataProvider _appProvider;
        winrt::TerminalApp::XamlMetaDataProvider AppProvider() { return _appProvider; };
    };

    template<typename D, typename... I>
    using AppT2 = App_baseWithProvider<D, I...>;
}


Review comment (suggestion):
So, the only thing we got from having it implemented like this was that it was lazy-created. Two questions:

1. If we're not lazy-creating it anymore, we can drop the function and just use `_appProvider` directly.
2. Does this result in a behavior change that might impact reliability? I'm moderately worried about that, but I bet that it was getting hit instantly on startup anyway...

Code after:
namespace winrt::TerminalApp::implementation
{
    template<typename D, typename... I>
    struct App_baseWithProvider : public App_base<D, ::winrt::Windows::UI::Xaml::Markup::IXamlMetadataProvider>
    {
        using IXamlType = ::winrt::Windows::UI::Xaml::Markup::IXamlType;

        IXamlType GetXamlType(::winrt::Windows::UI::Xaml::Interop::TypeName const& type)
        {
            return _appProvider.GetXamlType(type);
        }

        IXamlType GetXamlType(::winrt::hstring const& fullName)
        {
            return _appProvider.GetXamlType(fullName);
        }

        ::winrt::com_array<::winrt::Windows::UI::Xaml::Markup::XmlnsDefinition> GetXmlnsDefinitions()
        {
            return _appProvider.GetXmlnsDefinitions();
        }

    private:
        bool _contentLoaded{ false };
        winrt::TerminalApp::XamlMetaDataProvider _appProvider;
    };

    template<typename D, typename... I>
    using AppT2 = App_baseWithProvider<D, I...>;
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/PercentageSignConverter.h (C/C++)

Code before:

#include "PercentageSignConverter.g.h"
#include "Utils.h"

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    struct PercentageSignConverter : PercentageSignConverterT<PercentageSignConverter>
    {
        PercentageSignConverter() = default;

        Windows::Foundation::IInspectable Convert(Windows::Foundation::IInspectable const& value,
                                                  Windows::UI::Xaml::Interop::TypeName const& targetType,
                                                  Windows::Foundation::IInspectable const& parameter,
                                                  hstring const& language);

        Windows::Foundation::IInspectable ConvertBack(Windows::Foundation::IInspectable const& value,
                                                      Windows::UI::Xaml::Interop::TypeName const& targetType,
                                                      Windows::Foundation::IInspectable const& parameter,
                                                      hstring const& language);
    };
}

namespace winrt::Microsoft::Terminal::Settings::Editor::factory_implementation
{
    BASIC_FACTORY(PercentageSignConverter);
}


Review comment (suggestion):
```suggestion
DECLARE_CONVERTER(winrt::Microsoft::Terminal::Settings::Editor, PercentageSignConverter);
```

nit: here's a nice macro we have to make this easier :)

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

#include "PercentageSignConverter.g.h"
#include "Utils.h"

DECLARE_CONVERTER(winrt::Microsoft::Terminal::Settings::Editor, PercentageSignConverter);
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/PercentageSignConverter.cpp (C++)

Code before:
// Licensed under the MIT license.

#include "pch.h"
#include "PercentageSignConverter.h"
#include "PercentageSignConverter.g.cpp"

using namespace winrt::Windows;
using namespace winrt::Windows::UI::Xaml;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    Foundation::IInspectable PercentageSignConverter::Convert(Foundation::IInspectable const& value,
                                                              Windows::UI::Xaml::Interop::TypeName const& /* targetType */,
                                                              Foundation::IInspectable const& /* parameter */,
                                                              hstring const& /* language */)
    {
        const auto number{ winrt::unbox_value<double>(value) };
        return winrt::box_value(to_hstring((int)number) + L"%");
    }

    Foundation::IInspectable PercentageSignConverter::ConvertBack(Foundation::IInspectable const& value,
                                                                  Windows::UI::Xaml::Interop::TypeName const& /* targetType */,
                                                                  Foundation::IInspectable const& /*parameter*/,
                                                                  hstring const& /* language */)
    {
        return value;
    }
}


Review comment (suggestion):
```suggestion
        throw hresult_not_implemented();
```

1. Doesn't seem like we're using this
2. This doesn't seem correct. We want to convert an `hstring` of the format `#%` to an int or a double.

I'm ok with just throwing the not implemented hresult and calling it a day. 

Code after:
// Licensed under the MIT license.

#include "pch.h"
#include "PercentageSignConverter.h"
#include "PercentageSignConverter.g.cpp"

using namespace winrt::Windows;
using namespace winrt::Windows::UI::Xaml;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    Foundation::IInspectable PercentageSignConverter::Convert(Foundation::IInspectable const& value,
                                                              Windows::UI::Xaml::Interop::TypeName const& /* targetType */,
                                                              Foundation::IInspectable const& /* parameter */,
                                                              hstring const& /* language */)
    {
        const auto number{ winrt::unbox_value<double>(value) };
        return winrt::box_value(to_hstring((int)number) + L"%");
    }

    Foundation::IInspectable PercentageSignConverter::ConvertBack(Foundation::IInspectable const& /*value*/,
                                                                  Windows::UI::Xaml::Interop::TypeName const& /* targetType */,
                                                                  Foundation::IInspectable const& /*parameter*/,
                                                                  hstring const& /* language */)
    {
        throw hresult_not_implemented();
    }
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsModel/FontConfig.h (C/C++)

Code before:
  to the font settings of the terminal, for the terminal control.

Author(s):
- Pankaj Bhojwani - June 2021

--*/

#pragma once

#include "pch.h"
#include "FontConfig.g.h"
#include "JsonUtils.h"
#include "../inc/cppwinrt_utils.h"
#include "IInheritable.h"
#include <DefaultSettings.h>

namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
    struct FontConfig : FontConfigT<FontConfig>, IInheritable<FontConfig>
    {
    public:
        FontConfig(const winrt::weak_ref<Profile> sourceProfile);
        static winrt::com_ptr<FontConfig> CopyFontInfo(const winrt::com_ptr<FontConfig> source, const winrt::weak_ref<Profile> sourceProfile);
        Json::Value ToJson() const;
        void LayerJson(const Json::Value& json);
        bool HasAnyOptionSet();

        Model::Profile SourceProfile();

        INHERITABLE_SETTING(Model::FontConfig, hstring, FontFace, DEFAULT_FONT_FACE);
        INHERITABLE_SETTING(Model::FontConfig, int32_t, FontSize, DEFAULT_FONT_SIZE);
        INHERITABLE_SETTING(Model::FontConfig, Windows::UI::Text::FontWeight, FontWeight, DEFAULT_FONT_WEIGHT);

    private:
        winrt::weak_ref<Profile> _sourceProfile;
    };
}


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

Code after:
  to the font settings of the terminal, for the terminal control.

Author(s):
- Pankaj Bhojwani - June 2021

--*/

#pragma once

#include "pch.h"
#include "FontConfig.g.h"
#include "JsonUtils.h"
#include "../inc/cppwinrt_utils.h"
#include "IInheritable.h"
#include <DefaultSettings.h>

namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
    struct FontConfig : FontConfigT<FontConfig>, IInheritable<FontConfig>
    {
    public:
        FontConfig(winrt::weak_ref<Profile> sourceProfile);
        static winrt::com_ptr<FontConfig> CopyFontInfo(const winrt::com_ptr<FontConfig> source, winrt::weak_ref<Profile> sourceProfile);
        Json::Value ToJson() const;
        void LayerJson(const Json::Value& json);
        bool HasAnyOptionSet() const;

        Model::Profile SourceProfile();

        INHERITABLE_SETTING(Model::FontConfig, hstring, FontFace, DEFAULT_FONT_FACE);
        INHERITABLE_SETTING(Model::FontConfig, int32_t, FontSize, DEFAULT_FONT_SIZE);
        INHERITABLE_SETTING(Model::FontConfig, Windows::UI::Text::FontWeight, FontWeight, DEFAULT_FONT_WEIGHT);

    private:
        winrt::weak_ref<Profile> _sourceProfile;
    };
}