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

[Feature] ThrowHelper APIs with return values #3454

Closed
Sergio0694 opened this issue Aug 28, 2020 · 0 comments · Fixed by #3455
Closed

[Feature] ThrowHelper APIs with return values #3454

Sergio0694 opened this issue Aug 28, 2020 · 0 comments · Fixed by #3455
Assignees
Labels
Completed 🔥 feature request 📬 A request for new changes to improve functionality .NET Components which are .NET based (non UWP specific)
Milestone

Comments

@Sergio0694
Copy link
Member

Describe the problem this feature would solve

The new ThrowHelper APIs added in #3346 currently can't be used within switch statements or in cases where users need a return value from the invoked API in order to make the C# compiler happy. Consider this sample:

public static int Test_1(int x)
{
    return x switch
    {
        42 => 0,
        _ => throw new ArgumentException("Fail", nameof(x))
    };
}

That throw can't be replaced with a ThrowHelper call, as they all return void. This forces users to either manually write a throw function, which is verbose and defeats the whole point of having the ThrowHelper APIs, or to leave the exception there and get terrible codegen, which again defeats the whole point of worrying about this in the first place.

Describe the solution

We should also offer overloads of all the ThrowHelper APIs with a dummy T return, so that devs can use them in cases like this.
The above example would become as follows:

public static int Test_2(int x)
{
    return x switch
    {
        42 => 0,
        _ => ThrowHelper.ThrowArgumentException<int>("Fail", nameof(x))
    };
}

Which produces a very nice codegen:

C.Test_2(Int32)
    L0000: sub rsp, 0x28
    L0004: cmp ecx, 0x2a
    L0007: jne short L000d
    L0009: xor eax, eax
    L000b: jmp short L002c
    L000d: mov rcx, 0x23a7a40c000
    L0017: mov rcx, [rcx]
    L001a: mov rdx, 0x23a798da3a0
    L0024: mov rdx, [rdx]
    L0027: call ThrowHelper.ThrowArgumentException[[System.Int32, System.Private.CoreLib]](System.String, System.String)
    L002c: nop
    L002d: add rsp, 0x28
    L0031: ret

Describe alternatives you've considered

Additional context & Screenshots

Full sharplab.io sample here.

@Sergio0694 Sergio0694 added the feature request 📬 A request for new changes to improve functionality label Aug 28, 2020
@Sergio0694 Sergio0694 added this to the 7.0 milestone Aug 28, 2020
@Sergio0694 Sergio0694 self-assigned this Aug 28, 2020
@Sergio0694 Sergio0694 added .NET Components which are .NET based (non UWP specific) high-performance 🚂 Issues/PRs for the Microsoft.Toolkit.HighPerformance package labels Aug 28, 2020
@Sergio0694 Sergio0694 mentioned this issue Aug 28, 2020
7 tasks
@ghost ghost added the In-PR 🚀 label Aug 28, 2020
@ghost ghost closed this as completed in #3455 Sep 3, 2020
ghost pushed a commit that referenced this issue Sep 3, 2020
## Closes #3454
<!-- Add the relevant issue number after the "#" mentioned above (for ex: Fixes #1234) which will automatically close the issue once the PR is merged. -->

<!-- Add a brief overview here of the feature/bug & fix. -->

## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more that apply to this PR. -->

<!-- - Bugfix -->
 - Feature 
<!-- - Code style update (formatting) -->
<!-- - Refactoring (no functional changes, no api changes) -->
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## What is the new behavior?
<!-- Describe how was this issue resolved or changed? -->
New generic overloads for all `ThrowHelper` APIs, see linked issue for more details.

## PR Checklist

Please check if your PR fulfills the following requirements:

- [X] Tested code with current [supported SDKs](../readme.md#supported)
- [ ] Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->
- [ ] ~~Sample in sample app has been added / updated (for bug fixes / features)~~
    - [ ] ~~Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~
- [X] Tests for the changes have been added (for bug fixes / features) (if applicable)
- [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*)
- [X] Contains **NO** breaking changes
@ghost ghost added Completed 🔥 and removed In-PR 🚀 labels Sep 3, 2020
@Sergio0694 Sergio0694 removed the high-performance 🚂 Issues/PRs for the Microsoft.Toolkit.HighPerformance package label Sep 3, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Nov 2, 2020
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Completed 🔥 feature request 📬 A request for new changes to improve functionality .NET Components which are .NET based (non UWP specific)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant