Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp (C++)

Code before:
void CascadiaSettings::ExportFile(winrt::hstring path, winrt::hstring content)
{
    try
    {
        WriteUTF8FileAtomic({ path.c_str() }, til::u16u8(content));
    }
    CATCH_LOG();
}

void CascadiaSettings::_validateThemeExists()
{
    if (_globals->Themes().Size() == 0)
    {
        // We didn't even load the default themes. This should only be possible
        // if the defaults.json didn't include any themes, or if no
        // defaults.json was loaded at all. The second case is especially common
        // in tests (that don't bother with a defaults.json). No matter. Create
        // a default theme under `system` and just stick it in there.

        auto newTheme = winrt::make_self<Theme>();
        newTheme->Name(L"system");
        _globals->AddTheme(*newTheme);
        _globals->Theme(*winrt::make_self<ThemePair>(L"system"));
    }

    if (_globals->Theme().DarkName() == _globals->Theme().LightName())
    {
        // Only one theme. We'll treat it as such.
        if (!_globals->Themes().HasKey(_globals->Theme().DarkName()))
        {
            _warnings.Append(SettingsLoadWarnings::UnknownTheme);
            // safely fall back to system as the theme.
            _globals->Theme(*winrt::make_self<ThemePair>(L"system"));
        }
    }
    else
    {
        // Two different themes. Check each separately, and fall back to a
        // reasonable default contextually
        if (!_globals->Themes().HasKey(_globals->Theme().LightName()))
        {
            _warnings.Append(SettingsLoadWarnings::UnknownTheme);
            _globals->Theme().LightName(L"light");
        }
        if (!_globals->Themes().HasKey(_globals->Theme().DarkName()))
        {
            _warnings.Append(SettingsLoadWarnings::UnknownTheme);
            _globals->Theme().DarkName(L"dark");
        }
    }
}

Review comment (bug):
BTW I think it'd be advantageous for us in the long term if we consistently stopped calling WinRT functions repeatedly like that. These calls are about 4x as expensive as regular, non-inlined function calls and returning a runtimeclass reference involves at least 2 such calls each time. I realize that even then these calls are plenty fast for "cold paths" like this, but I think that it might lead to impacts down the road, for instance by impacting Debug build performance to a point where it begins to be annoying (Debug build WinRT calls are over 20x more expensive), or just because one of these function calls ends up not being `O(1)` anymore but e.g. `O(log N)` or something (an ordered map lookup instead of an unordered one for instance).

Just to be clear, I'm not saying that this is a problem right now, but I'm hoping to make a case for what I think might be beneficial for our long term code health.

Code after:
void CascadiaSettings::ExportFile(winrt::hstring path, winrt::hstring content)
{
    try
    {
        WriteUTF8FileAtomic({ path.c_str() }, til::u16u8(content));
    }
    CATCH_LOG();
}

void CascadiaSettings::_validateThemeExists()
{
    const auto& themes{ _globals->Themes() };
    if (themes.Size() == 0)
    {
        // We didn't even load the default themes. This should only be possible
        // if the defaults.json didn't include any themes, or if no
        // defaults.json was loaded at all. The second case is especially common
        // in tests (that don't bother with a defaults.json). No matter. Create
        // a default theme under `system` and just stick it in there.

        auto newTheme = winrt::make_self<Theme>();
        newTheme->Name(L"system");
        _globals->AddTheme(*newTheme);
        _globals->Theme(Model::ThemePair{ L"system" });
    }

    const auto& theme{ _globals->Theme() };
    if (theme.DarkName() == theme.LightName())
    {
        // Only one theme. We'll treat it as such.
        if (!themes.HasKey(theme.DarkName()))
        {
            _warnings.Append(SettingsLoadWarnings::UnknownTheme);
            // safely fall back to system as the theme.
            _globals->Theme(*winrt::make_self<ThemePair>(L"system"));
        }
    }
    else
    {
        // Two different themes. Check each separately, and fall back to a
        // reasonable default contextually
        if (!themes.HasKey(theme.LightName()))
        {
            _warnings.Append(SettingsLoadWarnings::UnknownTheme);
            theme.LightName(L"light");
        }
        if (!themes.HasKey(theme.DarkName()))
        {
            _warnings.Append(SettingsLoadWarnings::UnknownTheme);
            theme.DarkName(L"dark");
        }