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

Consider whether to switch IsSetInfo to use a class instead of a struct #32

Open
adamconnelly opened this issue Sep 15, 2020 · 0 comments
Labels
component/benchmarks Tasks relating to performance benchmarks. component/compiler

Comments

@adamconnelly
Copy link
Owner

The compiler generates an IsSetInfo struct containing a member for each optional field in the Thrift struct. At the moment it uses a C# struct rather than a class, to match what the official compiler does. The issue with this is that it breaks the guidelines for when to use a struct in C#.

Here's an example:

public class User
{
    private IsSetInfo isSet;
    private int? _Field1;

    /// <summary>
    /// Gets information about whether each optional field has been set.
    /// </summary>
    public IsSetInfo IsSet
    {
        get { return this.isSet; }
    }

    public int? Field1
    {
        get 
        {
            return _Field1;
        }

        set
        {
            this._Field1 = value;
            this.isSet.Field1 = true;
        }
    }

    /// <summary>
    /// Contains an entry for each optional field, indicating whether it has been
    /// set or not.
    /// </summary>
    public struct IsSetInfo
    {
        public bool Field1;
    }
}

Because of that, we should try to figure out why this decision was made, and if it makes sense to keep it as a struct, we should document the reason why. In case a struct was chosen for performance reasons, we should wait to make any changes until we have benchmarks in place so we can justify the change makes sense.

@adamconnelly adamconnelly added component/compiler component/benchmarks Tasks relating to performance benchmarks. labels Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/benchmarks Tasks relating to performance benchmarks. component/compiler
Projects
None yet
Development

No branches or pull requests

1 participant