Skip to content

v: fix mutable option#19100

Merged
spytheman merged 32 commits into
vlang:masterfrom
felipensp:fix_opt_mut
Jun 5, 2025
Merged

v: fix mutable option#19100
spytheman merged 32 commits into
vlang:masterfrom
felipensp:fix_opt_mut

Conversation

@felipensp

@felipensp felipensp commented Aug 9, 2023

Copy link
Copy Markdown
Member

Fix #18818
Fix #24622
Fix #24101

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
Copy Markdown
Contributor

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

Comment thread vlib/v/parser/fn.v Outdated
Comment thread vlib/v/ast/types.v Outdated
generic
shared_f
atomic_f
option_mut_param_t

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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

@felipensp

Copy link
Copy Markdown
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
@spytheman

Copy link
Copy Markdown
Contributor

Closing for inactivity.
The master branch diverged a lot from this, and I do not see a good way forward for it, that is more efficient than just re-implementing the changes here over the current master.

@spytheman spytheman closed this Apr 9, 2025
@felipensp felipensp reopened this May 31, 2025
@huly-for-github huly-for-github Bot closed this May 31, 2025
@felipensp felipensp reopened this May 31, 2025
@JalonSolov

Copy link
Copy Markdown
Collaborator

Lack of running v fmt strikes again. :-)

@spytheman

Copy link
Copy Markdown
Contributor

Is it ready for review now?

@felipensp felipensp marked this pull request as ready for review June 3, 2025 12:06
@felipensp

Copy link
Copy Markdown
Member Author

Is it ready for review now?

Yes.

Comment thread vlib/v/tests/options/option_mut_generic_array_test.v Outdated
Comment thread vlib/v/tests/options/option_mut_struct_init_test.v
Comment thread vlib/v/tests/options/option_mut_struct_init_test.v Outdated
Comment thread vlib/v/gen/native/tests/struct.vv
Comment thread vlib/v/gen/native/tests/string.vv
Comment thread vlib/v/gen/c/cgen.v Outdated
Comment thread vlib/v/gen/c/cgen.v Outdated
@spytheman spytheman merged commit e4e5689 into vlang:master Jun 5, 2025
74 checks passed
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

3 participants