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

v: fix mutable option #19100

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

v: fix mutable option #19100

wants to merge 7 commits into from

Conversation

felipensp
Copy link
Member

@felipensp felipensp commented Aug 9, 2023

Fix #18818

struct Abc {
        a int 
}

fn foo(mut baz ?Abc) {
        baz = Abc{a:3}
        println(baz)
        dump(baz)
}

fn main() {
	mut a := ?Abc{
                a:2
        }
	dump(a)
        foo(mut a)
        println('--')
        dump(a)
}

🤖 Generated by Copilot at 7d84ddf

This pull request fixes the C code generation for option types that are passed as mutable parameters to functions. It adds a new type flag option_mut_param_t to mark these types and adjusts the C type names, assignments, and string representations accordingly. It also fixes some bugs in the code generation for option expressions and function calls.

🤖 Generated by Copilot at 7d84ddf

  • Add a new type flag option_mut_param_t to mark option types that are passed as mutable parameters or return values (link)
    • Use memcpy to copy the right expression to the data field of the option struct in assignments (link, link, link)
    • Remove the asterisk from the C type name and the variable declaration, since the option struct already contains a pointer to the data type (link, link, link, link)
    • Skip the double dereference or the reference of the option value, depending on the context, since the option struct already contains a pointer to the data type (link, link, link, link, link)
    • Add a dereference to the option value in the dump_expr method, which is used for debugging and error reporting (link)
  • Modify the parse_param and parse_return_type methods in vlib/v/parser/fn.v to set the option_mut_param_t flag on option types without any pointer modifiers (link, link)
  • Skip the generation of option types that end with an asterisk in the gen_option_type method in vlib/v/gen/c/cgen.v, since they are already handled by the option_mut_param_t flag (link)

@spytheman
Copy link
Member

./v -autofree -o v2 cmd/v is failing for some reason.

@@ -113,6 +113,7 @@ pub enum TypeFlag {
generic
shared_f
atomic_f
option_mut_param_t
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need that? mut is usually just a modifier, not something that is part of the type.

Copy link
Member

Choose a reason for hiding this comment

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

TypeFlags are limited to just 3 bits, i.e. just 8 combinations :-|

Copy link
Member

Choose a reason for hiding this comment

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

@joe-conigliaro can you please also review this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you really need that? mut is usually just a modifier, not something that is part of the type.

Yes. I was thinking a way to determine when an option ptr must be used as struct _option_*_ptr or a real pointer.

@spytheman
Copy link
Member

The CI is passing, can it be reviewed/merged now?

@felipensp
Copy link
Member Author

The CI is passing, can it be reviewed/merged now?

It was missing a parameter validation, checking if everything is ok yet.

@JalonSolov JalonSolov added the Needs Rebase The code of the PR must be rebased over current master before it can be approved. label Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Rebase The code of the PR must be rebased over current master before it can be approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

foo(mut a) generates invalid c code, for fn foo(mut baz ?&Abc) {, and mut a := ?Abc{}
3 participants