ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/terminal
File: src/inc/til/winrt.h (C/C++)

Code before:
        property<T>() = default;
        property<T>(T defaultValue) :
            _value{ defaultValue } {}

        T operator()() const
        {
            return _value;
        }
        void operator()(const T& value)
        {
            _value = value;
        }
        void operator()(const T&& value)
        {
            _value = value;
        }
        property<T>& operator=(const property<T>& other)
        {
            _value = other._value;
            return *this;
        }
        property<T>& operator=(const T& newValue)
        {
            _value = newValue;
            return *this;
        }

    private:
        T _value;
    };

#ifdef WINRT_Windows_Foundation_H

    template<typename ArgsT>
    struct event
    {
        event<ArgsT>() = default;
        winrt::event_token operator()(const ArgsT& handler) { return _handlers.add(handler); }
        void operator()(const winrt::event_token& token) { _handlers.remove(token); }
        template<typename... Arg>
        void raise(Arg const&... args)
        {
            _handlers(args...);
        }
        winrt::event<ArgsT> _handlers;
    };

    template<typename ArgsT>
    struct forwarded_event
    {
        forwarded_event<ArgsT>() = delete;

Review comment (suggestion):
nit: You can just `= default` all these. By usual convention, such operators and constructors should be right after the 2 constructors at the top.

Code after:
        explicit constexpr property(auto&&... args) :
            _value{ std::forward<decltype(args)>(args)... } {}

        property& operator=(const property& other) = default;

        T operator()() const
        {
            return _value;
        }
        void operator()(auto&& arg)
        {
            _value = std::forward<decltype(arg)>(arg);
        }
        operator bool() const noexcept
        {
            if constexpr (std::is_same_v<T, winrt::hstring>)
            {
                return !_value.empty();
            }
            else
            {
                return _value;
            }
        }
        bool operator==(const property& other) const noexcept
        {
            return _value == other._value;
        }
        bool operator!=(const property& other) const noexcept
        {
            return _value != other._value;
        }
        bool operator==(const T& other) const noexcept
        {
            return _value == other;
        }
        bool operator!=(const T& other) const noexcept
        {
            return _value != other;
        }

    private:
        T _value;
    };

#ifdef WINRT_Windows_Foundation_H

    template<typename ArgsT>
    struct event
    {
        event<ArgsT>() = default;
Repository: microsoft/terminal
File: src/cascadia/UnitTests_TerminalCore/TilWinRtHelpersTests.cpp (C++)

Code before:

void TilWinRtHelpersTests::TestPropertySimple()
{
    til::property<int> Foo;
    til::property<int> Bar(11);

    VERIFY_ARE_EQUAL(11, Bar());

    Foo(42);
    VERIFY_ARE_EQUAL(42, Foo());

    Foo = Foo() - 5; // 37
    VERIFY_ARE_EQUAL(37, Foo());

    Foo = Foo() + Bar(); // 48
    VERIFY_ARE_EQUAL(48, Foo());
}

void TilWinRtHelpersTests::TestPropertyHString()
{
    til::property<winrt::hstring> Foo{ L"Foo" };

    VERIFY_ARE_EQUAL(L"Foo", Foo());

    Foo = L"bar";
    VERIFY_ARE_EQUAL(L"bar", Foo());
}

void TilWinRtHelpersTests::TestTruthiness()
{
    til::property<bool> Foo{ false };
    til::property<int> Bar(0);
    til::property<winrt::hstring> EmptyString;
    til::property<winrt::hstring> FullString{ L"Full" };

    VERIFY_IS_FALSE(Foo());
    VERIFY_IS_FALSE(Foo);

    VERIFY_IS_FALSE(Bar());
    VERIFY_IS_FALSE(Bar);

    VERIFY_IS_FALSE(EmptyString);
    VERIFY_IS_FALSE(!EmptyString().empty());

    Foo = true;
    VERIFY_IS_TRUE(Foo());
    VERIFY_IS_TRUE(Foo);

    Bar = 11;
    VERIFY_IS_TRUE(Bar());
    VERIFY_IS_TRUE(Bar);

Review comment (suggestion):
what if you compare...

```
VERIFY_ARE_EQUAL(L"bar", Foo); // without the function call operator
```

Code after:

void TilWinRtHelpersTests::TestPropertySimple()
{
    til::property<int> Foo;
    til::property<int> Bar(11);

    VERIFY_ARE_EQUAL(11, Bar());

    Foo(42);
    VERIFY_ARE_EQUAL(42, Foo());

    Foo(Foo() - 5); // 37
    VERIFY_ARE_EQUAL(37, Foo());

    Foo(Foo() + Bar()); // 48
    VERIFY_ARE_EQUAL(48, Foo());
}

void TilWinRtHelpersTests::TestPropertyHString()
{
    til::property<winrt::hstring> Foo{ L"Foo" };

    VERIFY_ARE_EQUAL(L"Foo", Foo());

    Foo(L"bar");
    VERIFY_ARE_EQUAL(L"bar", Foo());
}

void TilWinRtHelpersTests::TestTruthiness()
{
    til::property<bool> Foo{ false };
    til::property<int> Bar(0);
    til::property<winrt::hstring> EmptyString;
    til::property<winrt::hstring> FullString{ L"Full" };

    VERIFY_IS_FALSE(Foo());
    VERIFY_IS_FALSE(Foo);

    VERIFY_IS_FALSE(Bar());
    VERIFY_IS_FALSE(Bar);

    VERIFY_IS_FALSE(EmptyString);
    VERIFY_IS_FALSE(!EmptyString().empty());

    Foo(true);
    VERIFY_IS_TRUE(Foo());
    VERIFY_IS_TRUE(Foo);

    Bar(11);
    VERIFY_IS_TRUE(Bar());
    VERIFY_IS_TRUE(Bar);
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"
{
    namespace details
    {
        template<typename T>
        constexpr T winrt_empty_value() noexcept
        {
            if constexpr (std::is_base_of_v<winrt::Windows::Foundation::IUnknown, T>)
            {
                return nullptr;
            }
            else
            {
                return {};
            }
        }
    };

    template<typename T>
    struct property
    {
        property<T>(const T& defaultValue = details::winrt_empty_value<T>()) :
            _value{ defaultValue } {}

        property<T>& operator=(const property<T>& other) = default;

        T operator()() const
        {
            return _value;
        }
        void operator()(const T& value)
        {
            _value = value;
        }
        property<T>& operator=(const T& newValue)
        {

Review comment (suggestion):
Perhaps @lhecker knows better, but I think this might cause a copy sometimes? and that making it `T const&` would not do that?

I don't know which is cheaper.

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) :
            _value{ std::forward<decltype(args)>(args)... } {}

        property& operator=(const property& other) = default;

        T operator()() const
        {
            return _value;
        }
        void operator()(auto&& arg)
        {
            _value = std::forward<decltype(arg)>(arg);
        }
        operator bool() const noexcept
        {
            if constexpr (std::is_same_v<T, winrt::hstring>)
            {
                return !_value.empty();
            }
            else
            {
                return _value;
            }
        }
        bool operator==(const property& other) const noexcept
        {
            return _value == other._value;
        }
        bool operator!=(const property& other) const noexcept
        {
            return _value != other._value;
Repository: microsoft/terminal
File: src/inc/til/winrt.h (C/C++)

Code before:

#pragma once

namespace til // Terminal Implementation Library. Also: "Today I Learned"
{
    namespace details
    {
        template<typename T>
        constexpr T winrt_empty_value() noexcept
        {
            if constexpr (std::is_base_of_v<winrt::Windows::Foundation::IUnknown, T>)
            {
                return nullptr;
            }
            else
            {
                return {};
            }
        }
    };

    template<typename T>
    struct property
    {
        property<T>(const T& defaultValue = details::winrt_empty_value<T>()) :
            _value{ defaultValue } {}

        property<T>& operator=(const property<T>& other) = default;

        T operator()() const
        {
            return _value;
        }
        void operator()(const T& value)
        {
            _value = value;
        }
        property<T>& operator=(const T& newValue)
        {
            _value = newValue;
            return *this;
        }
        operator bool() const requires std::is_same_v<T, winrt::hstring>
        {
            return !_value.empty();
        }
        operator bool() const requires std::convertible_to<T, bool>
        {
            return _value;
        }
        bool operator==(const property<T>& other) const

Review comment (suggestion):
```suggestion
        explicit constexpr property(auto&&... args) :
            _value{ std::forward<decltype(args)>(args)... } {}
```

This should most likely allow you to drop `winrt_empty_value`, as well as the `.c_str()` in `ControlProperties.h`.

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

Code before:

namespace til // Terminal Implementation Library. Also: "Today I Learned"
{
    namespace details
    {
        template<typename T>
        constexpr T winrt_empty_value() noexcept
        {
            if constexpr (std::is_base_of_v<winrt::Windows::Foundation::IUnknown, T>)
            {
                return nullptr;
            }
            else
            {
                return {};
            }
        }
    };

    template<typename T>
    struct property
    {
        property<T>(const T& defaultValue = details::winrt_empty_value<T>()) :
            _value{ defaultValue } {}

        property<T>& operator=(const property<T>& other) = default;

        T operator()() const
        {
            return _value;
        }
        void operator()(const T& value)
        {
            _value = value;
        }
        property<T>& operator=(const T& newValue)
        {
            _value = newValue;
            return *this;
        }
        operator bool() const requires std::is_same_v<T, winrt::hstring>
        {
            return !_value.empty();
        }
        operator bool() const requires std::convertible_to<T, bool>
        {
            return _value;
        }
        bool operator==(const property<T>& other) const
        {
            return _value == other._value;

Review comment (suggestion):
You don't need to explicitly write `<T>` inside the template. You do need to declare the other 3 standard operators and constructors though, or otherwise AuditMode will complain. BTW I think our AuditMode isn't working for TerminalControl once again. 😅

Code after:

namespace til // Terminal Implementation Library. Also: "Today I Learned"
{
    template<typename T>
    struct property
    {
        explicit constexpr property(auto&&... args) :
            _value{ std::forward<decltype(args)>(args)... } {}

        property& operator=(const property& other) = default;

        T operator()() const
        {
            return _value;
        }
        void operator()(auto&& arg)
        {
            _value = std::forward<decltype(arg)>(arg);
        }
        operator bool() const noexcept
        {
            if constexpr (std::is_same_v<T, winrt::hstring>)
            {
                return !_value.empty();
            }
            else
            {
                return _value;
            }
        }
        bool operator==(const property& other) const noexcept
        {
            return _value == other._value;
        }
        bool operator!=(const property& other) const noexcept
        {
            return _value != other._value;
        }
        bool operator==(const T& other) const noexcept
        {
            return _value == other;
        }
        bool operator!=(const T& other) const noexcept
        {
            return _value != other;
        }

    private:
        T _value;
    };
Repository: microsoft/terminal
File: src/inc/til/winrt.h (C/C++)

Code before:
            {
                return nullptr;
            }
            else
            {
                return {};
            }
        }
    };

    template<typename T>
    struct property
    {
        property<T>(const T& defaultValue = details::winrt_empty_value<T>()) :
            _value{ defaultValue } {}

        property<T>& operator=(const property<T>& other) = default;

        T operator()() const
        {
            return _value;
        }
        void operator()(const T& value)
        {
            _value = value;
        }
        property<T>& operator=(const T& newValue)
        {
            _value = newValue;
            return *this;
        }
        operator bool() const requires std::is_same_v<T, winrt::hstring>
        {
            return !_value.empty();
        }
        operator bool() const requires std::convertible_to<T, bool>
        {
            return _value;
        }
        bool operator==(const property<T>& other) const
        {
            return _value == other._value;
        }
        bool operator!=(const property<T>& other) const
        {
            return _value != other._value;
        }
        bool operator==(const T& other) const
        {
            return _value == other;
        }

Review comment (suggestion):
```suggestion
        void operator()(auto&& arg)
        {
            _value = std::forward<decltype(arg)>(arg);
        }
```

Code after:
        property& operator=(const property& other) = default;

        T operator()() const
        {
            return _value;
        }
        void operator()(auto&& arg)
        {
            _value = std::forward<decltype(arg)>(arg);
        }
        operator bool() const noexcept
        {
            if constexpr (std::is_same_v<T, winrt::hstring>)
            {
                return !_value.empty();
            }
            else
            {
                return _value;
            }
        }
        bool operator==(const property& other) const noexcept
        {
            return _value == other._value;
        }
        bool operator!=(const property& other) const noexcept
        {
            return _value != other._value;
        }
        bool operator==(const T& other) const noexcept
        {
            return _value == other;
        }
        bool operator!=(const T& other) const noexcept
        {
            return _value != other;
        }

    private:
        T _value;
    };

#ifdef WINRT_Windows_Foundation_H

    template<typename ArgsT>
    struct event
    {
        event<ArgsT>() = default;
        winrt::event_token operator()(const ArgsT& handler) { return _handlers.add(handler); }
        void operator()(const winrt::event_token& token) { _handlers.remove(token); }
        operator bool() const noexcept { return _handlers; }
Repository: microsoft/terminal
File: src/cascadia/TerminalApp/TabBase.cpp (C++)

Code before:
            L"TabViewButtonForegroundPointerOver"
        };

        const auto& tabItemResources{ TabViewItem().Resources() };

        // simply clear any of the colors in the tab's dict
        for (const auto& keyString : keys)
        {
            auto key = winrt::box_value(keyString);
            if (tabItemResources.HasKey(key))
            {
                tabItemResources.Remove(key);
            }
        }

        // GH#11382 DON'T set the background to null. If you do that, then the
        // tab won't be hit testable at all. Transparent, however, is a totally
        // valid hit test target. That makes sense.
        TabViewItem().Background(WUX::Media::SolidColorBrush{ Windows::UI::Colors::Transparent() });

        _RefreshVisualState();
    }

    // Method Description:
    // - Toggles the requested theme of the tab view item,
    //   so that changes to the tab color are reflected immediately
    // - Prior to MUX 2.8, we toggled the visual state here, but that seemingly
    //   doesn't work in 2.8.
    // Arguments:
    // - <none>
    // Return Value:
    // - <none>
    void TabBase::_RefreshVisualState()
    {
        const auto& reqTheme = TabViewItem().RequestedTheme();
        TabViewItem().RequestedTheme(ElementTheme::Light);
        TabViewItem().RequestedTheme(ElementTheme::Dark);
        TabViewItem().RequestedTheme(reqTheme);
    }
}


Review comment (suggestion):
```suggestion
    // BODGY
    // - Toggles the requested theme of the tab view item,
```

Code after:
            L"TabViewButtonForegroundPointerOver"
        };

        const auto& tabItemResources{ TabViewItem().Resources() };

        // simply clear any of the colors in the tab's dict
        for (const auto& keyString : keys)
        {
            auto key = winrt::box_value(keyString);
            if (tabItemResources.HasKey(key))
            {
                tabItemResources.Remove(key);
            }
        }

        // GH#11382 DON'T set the background to null. If you do that, then the
        // tab won't be hit testable at all. Transparent, however, is a totally
        // valid hit test target. That makes sense.
        TabViewItem().Background(WUX::Media::SolidColorBrush{ Windows::UI::Colors::Transparent() });

        _RefreshVisualState();
    }

    // Method Description:
    // BODGY
    // - Toggles the requested theme of the tab view item,
    //   so that changes to the tab color are reflected immediately
    // - Prior to MUX 2.8, we toggled the visual state here, but that seemingly
    //   doesn't work in 2.8.
    // Arguments:
    // - <none>
    // Return Value:
    // - <none>
    void TabBase::_RefreshVisualState()
    {
        const auto& reqTheme = TabViewItem().RequestedTheme();
        TabViewItem().RequestedTheme(ElementTheme::Light);
        TabViewItem().RequestedTheme(ElementTheme::Dark);
        TabViewItem().RequestedTheme(reqTheme);
    }
}
Repository: microsoft/terminal
File: src/inc/til/generational.h (C/C++)

Code before:

        constexpr bool operator==(const generational& rhs) const noexcept { return generation() == rhs.generation(); }
        constexpr bool operator!=(const generational& rhs) const noexcept { return generation() != rhs.generation(); }

        constexpr generation_t generation() const noexcept
        {
            return _generation;
        }

        [[nodiscard]] constexpr const T* operator->() const noexcept
        {
            return &_value;
        }

        [[nodiscard]] constexpr const T& operator*() const noexcept
        {
            return _value;
        }

        [[nodiscard]] constexpr T* write() noexcept
        {
            _generation.bump();
            return &_value;
        }

    private:
        generation_t _generation;
        T _value;
    };
}


Review comment (suggestion):
it seems weird to my brain that these return different types, but I'm sure this is just how c++ is under the hood and I never really thought about it

Code after:
    // invalidated than was necessary, but on the other hand it makes both,
    // the code simpler and the fast-path (no state change) much faster.
    template<typename T>
    struct generational
    {
        generational() = default;
        explicit constexpr generational(auto&&... args) :
            _value{ std::forward<decltype(args)>(args)... } {}
        explicit constexpr generational(generation_t generation, auto&&... args) :
            _generation{ generation },
            _value{ std::forward<decltype(args)>(args)... } {}

        constexpr bool operator==(const generational& rhs) const noexcept { return generation() == rhs.generation(); }
        constexpr bool operator!=(const generational& rhs) const noexcept { return generation() != rhs.generation(); }

        constexpr generation_t generation() const noexcept
        {
            return _generation;
        }

        [[nodiscard]] constexpr const T* operator->() const noexcept
        {
            return &_value;
        }

        [[nodiscard]] constexpr const T& operator*() const noexcept
        {
            return _value;
        }

        [[nodiscard]] constexpr T* write() noexcept
        {
            _generation.bump();
            return &_value;
        }

    private:
        generation_t _generation;
        T _value{};
    };
}