Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The new variables in winrt::impl::consume_ methods can collide with parameters to that method #1455

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

dmachaj
Copy link
Contributor

@dmachaj dmachaj commented Nov 20, 2024

Why is this change being made?

@DefaultRyan discovered a subtle compatibility issue with the cast result checking changes and certain projected methods. The variables in the consume_ method, such as the generically-named code variable, can shadow the parameters to the function. This led to a build break where a function took an int16_t named code and that was shadowed by the int32_t named code with the HRESULT in it. Fortunately the compiler had our back and flagged the size truncation as a build break so it was noticed.

Briefly summarize what changed

The variable names in these generated functions are now much uglier (and therefore less likely to collide by coincidence). They have an underscore prefix and then lowercase which should put them into an unofficial namespace that shouldn't collide with anything else. The code variable is now named _winrt_cast_result_code for example. While I was renaming everything I realized that my previous names mismatched the cppwinrt naming convention of snake_case so I fixed them up.

How was this change tested?

build_test_all.cmd passed.

Here is the new code gen for Windows::Foundation::ToString()

    template <typename D> auto consume_Windows_Foundation_IStringable<D>::ToString() const
    {
        void* value{};
        if constexpr (!std::is_same_v<D, winrt::Windows::Foundation::IStringable>)
        {
            auto const [_winrt_casted_result, _winrt_cast_result_code] = impl::try_as_with_reason<winrt::Windows::Foundation::IStringable, D const*>(static_cast<D const*>(this));
            check_hresult(_winrt_cast_result_code);
            auto const _winrt_abi_type = *(abi_t<winrt::Windows::Foundation::IStringable>**)&_winrt_casted_result;
            check_hresult(_winrt_abi_type->ToString(&value));
        }
        else
        {
            auto const _winrt_abi_type = *(abi_t<winrt::Windows::Foundation::IStringable>**)this;
            check_hresult(_winrt_abi_type->ToString(&value));
        }
        return hstring{ value, take_ownership_from_abi };
    }

@dmachaj dmachaj merged commit fa079fb into master Nov 20, 2024
75 checks passed
@dmachaj dmachaj deleted the user/dmachaj/consume-variable-collisions branch November 20, 2024 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants