-
Notifications
You must be signed in to change notification settings - Fork 210
Fe test cmd #1201
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
Fe test cmd #1201
Conversation
|
Yes, thanks. I'll address that.
…On Wed, 7 Jan 2026 at 23:09, sbillig ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In crates/codegen/src/yul/emitter/function.rs
<#1201 (comment)>:
> @@ -85,6 +95,23 @@ impl<'db> FunctionEmitter<'db> {
/// Returns true if the Fe function has a return type.
pub(super) fn returns_value(&self) -> bool {
let ret_ty = self.mir_func.func.return_ty(self.db);
+ // Substitute generic parameters with their concrete arguments for monomorphized functions
This shouldn't be happening in codegen; MIR should be providing a fully
resolved type. The codegen crate shouldn't know anything about type
folding, etc.
—
Reply to this email directly, view it on GitHub
<#1201 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7HFNNENAOAINE3OBIJRL4FWG3LAVCNFSM6AAAAACQTUZ32CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMMZXGE2DIMBTHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
@cburgdorf I pushed up a fix for this, along with rebasing on master. Feel free to merge this if you're ready (I'll just close my create2 pr). |
|
I'm working on rebasing my other stuff on top of this, btw. |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43c1b0ca73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let init_off = C::init_code_offset() | ||
| let args_len = encoded_size<C::InitArgs>() | ||
| let total_len = init_len + args_len | ||
|
|
||
| // Build `initcode || abi.encode(args)` contiguously. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Account for dynamic ABI sizes in create()
The buffer length for constructor args is derived from encoded_size<C::InitArgs>(), which is just the static AbiSize constant. If an Encode impl uses append_tail to encode dynamic content (e.g., user-defined ABI types or future std types), the actual encoded length can exceed args_len, but total_len (and therefore the CREATE input) stays fixed at init_len + args_len. That will truncate the encoded args and lead to incorrect constructor decoding or deployment reverts. Consider either enforcing InitArgs to be statically sized only, or compute the real length via the encoder’s finish()/tail allocation before calling create_raw (same issue applies to create2).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is right, but we don't support encoding any dynamically-sized things yet. I'll create an issue.
2f2ce9e to
43c1b0c
Compare
Sorry, I read this a little late and had force pushed your fix away with my own fix. I restored your fix now and will merge this. |
No description provided.