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

rewriter: wrap AbiSignature in FnSignature that also stores API info (arg/return types) #514

Open
wants to merge 2 commits into
base: kkysen/multi-conditions
Choose a base branch
from

Conversation

kkysen
Copy link
Contributor

@kkysen kkysen commented Feb 24, 2025

This wraps AbiSignature in a more generic FnSignature that also contains API info as well as the existing ABI info. The API info is stored in ApiSignature, which contains Params for the args and return. Each Param contains the parameter name (if a parameter), the type name, the canonical type name, and a type ID based on the canonical name. For now, the type ID is counted by interning the canonical types, so it's unique within a single ia2-rewriter invocation.

Next, we can use this API info to both define C functions (if we want to) and call into the type-registry library (#493) with type IDs for the arguments and return type.

Copy link
Contributor

@randomPoison randomPoison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seem to be a lot of unrelated formatting changes in here. Are you making these changes manually or is this formatting style being enforced by clang tidy or something? Either way it'd be better to split the formatting changes into their own PR, rather than lumping them in here with other functional changes.

@kkysen
Copy link
Contributor Author

kkysen commented Feb 25, 2025

There seem to be a lot of unrelated formatting changes in here. Are you making these changes manually or is this formatting style being enforced by clang tidy or something? Either way it'd be better to split the formatting changes into their own PR, rather than lumping them in here with other functional changes.

There are a lot of files that aren't formatted (nothing in CI checks this), but it's convenient to autoformat when I'm editing, so I formatted these files that I'm editing here. I did split them into separate commits, but I wasn't sure if a whole PR is worth it. But I can do that; it's pretty trivial.

@randomPoison
Copy link
Contributor

I did split them into separate commits, but I wasn't sure if a whole PR is worth it. But I can do that; it's pretty trivial.

Nah, don't worry about it here. I would just try to do separate PRs for any non-trivial formatting in the future.

Copy link
Contributor

@randomPoison randomPoison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, though I'm not familiar enough with the rewriter to know if this is the best solution. I defer to @ayrtonm and @fw-immunant to judge that.

kkysen added 2 commits March 2, 2025 23:45
This is meant to store other non-strictly ABI info
about the function's signature, which stays in `AbiSignature`.
More specifically, it stores the parameter names,
parameter types, and return type.
For the types, it stores the type name, the canonical type name, and a type ID.
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.

2 participants