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

Simplify TypeContext #3883

Open
bernardnormier opened this issue Jan 22, 2024 · 3 comments
Open

Simplify TypeContext #3883

bernardnormier opened this issue Jan 22, 2024 · 3 comments

Comments

@bernardnormier
Copy link
Member

The TypeContext enum is too complicated:

pub enum TypeContext {
    /// Used when generating the types of fields in structs, classes, and exceptions.
    Field,
    /// Used when generating the types of operation parameters, and return types in places where
    /// they're being decoded.
    Decode,
    /// Used when generating the types of operation parameters, and return types in places where
    /// they're being encoded.
    Encode,
    /// Used when generating types that are parts of other types, such as the success & failure types of results,
    /// the key & value types of dictionaries, or the element type of a sequence.
    Nested,
}

First, we should always treat Field and Nested the same, so there is no need for 2 enumerators. It should always be Field.
Then, Encode/Decode are confusing. I propose to rename them IncomingParam / OutgoingParam.

@bernardnormier
Copy link
Member Author

bernardnormier commented Jan 23, 2024

We should also review how we use TypeContext.

It looks like we use it for at least 2 separate purposes:

  1. in cs_type_string, to figure out how to map a type based on the "context" (which seems fine)
  2. in encode_type and friends, for the special handling for sequence parameters; this does not seem correct to me.

@InsertCreativityHere
Copy link
Member

The original proposal was implemented in 2685acc.

But I agree, more simplification seems possible here.

@InsertCreativityHere
Copy link
Member

More cleanup has been done, the biggest being #3893.
Now, we no longer use TypeContext for the functions that have replaced cs_type_string.

Now it's really only used in the proxy and dispatch generation code, and the functions that it calls into.
But, there's room for extra improvement, since the proxy side only uses OutgoingParam and Field.
And the dispatch side only uses IncomingParam and Field.

So in reality, it behaves more like a bool, and we only need 2 of it's values at any given time.

@InsertCreativityHere InsertCreativityHere modified the milestones: 0.3.0, Future Feb 13, 2024
@InsertCreativityHere InsertCreativityHere removed their assignment Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants