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

comptime: support -d ident=value and var := $d('ident', 0) #21685

Merged
merged 26 commits into from
Jun 19, 2024

Conversation

larpon
Copy link
Contributor

@larpon larpon commented Jun 16, 2024

Updated: Initial suggestion was to use -cv ident=value and $compile_value('ident', <value>), it is now an extension of -d using $d('ident', <value>) as retriever.

This PR adds support for the following:

v -d my_i64=890 -d my_string="five six" run .

const my_i64 = $d('my_i64',4)
const my_string = $d('my_string','five')

fn main() {
    println(my_i64)
    println(my_string)
}

Resulting in:

890
five six

.. and leaving out the -d flags:
v run ., outputs:

4
five

Supported types are "pure" literals, and can be used also without const:

my_f64 := $d('my_f64', 1.0)
my_i64 := $d('my_i64', 2)
my_string := $d('my_string', 'three')
my_bool := $d('my_bool', false)
my_char := $d('my_char', `f`)

I'm suggesting this, primarily, because I need to be able to tweak fixed array sizes at compile time - so that they have a default, but so I can decide to let my users tweak the sizes if they have reasons or needs to do so.

As @spytheman mentioned on Discord; It has potential to replace both (current) -d and $env.

I plan on adding support for #flag -I $d('my_flag','flag_value')/xyz and #include "$d('my_include','/usr/include')/stdio.h" when time permits. (DONE)

Copy link
Contributor

@StunxFS StunxFS left a comment

Choose a reason for hiding this comment

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

Great work!

.github/workflows/comptime_ci.yml Outdated Show resolved Hide resolved
.github/workflows/comptime_ci.yml Outdated Show resolved Hide resolved
larpon and others added 2 commits June 16, 2024 15:25
Co-authored-by: Jose Mendoza  <56417208+StunxFS@users.noreply.github.com>
Co-authored-by: Jose Mendoza  <56417208+StunxFS@users.noreply.github.com>
vlib/v/pref/pref.v Outdated Show resolved Hide resolved
vlib/v/pref/pref.v Outdated Show resolved Hide resolved
vlib/v/fmt/fmt.v Outdated Show resolved Hide resolved
@spytheman
Copy link
Member

It is an excellent and long needed addition.
Great work!

@larpon
Copy link
Contributor Author

larpon commented Jun 16, 2024

Awesome, thanks @spytheman

@enghitalo
Copy link
Contributor

What you guys think about use --compile-values instead of -cv flag? Or using both?

@larpon larpon marked this pull request as ready for review June 16, 2024 19:11
@JalonSolov
Copy link
Contributor

It would be -compile-values - V only uses 1 -.

The longer name would be more explicit, for sure. -cv could be almost anything... Want V to write a Ciriculam Vitae for you? -cv. :-)

@larpon
Copy link
Contributor Author

larpon commented Jun 16, 2024

What you guys think about use --compile-values instead of -cv flag? Or using both?

Currently most, if not all, v flags is denoted via single dash -.
The PR supports both a short and long version:
-cv and -compile-value

(and you can even use them multiple times interchangeable: v -cv some_var=4 -compile-value xyz=4.7)

Using the plural version (with "s"; -compile-values) when multiple flags/args can be supplied is not ideal.

Copy link
Member

@spytheman spytheman left a comment

Choose a reason for hiding this comment

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

@medvednikov what do you think about this new language feature?

@medvednikov
Copy link
Member

I like it.

But we do have -d compvar, which does the same thing, so it would probably have to be removed for "one way"?

@medvednikov
Copy link
Member

(well, not the same, -d is a subset of this new feature)

@larpon
Copy link
Contributor Author

larpon commented Jun 17, 2024

If we decide to remove -d it probably needs an extended deprecation period since it's a very "old" and very frequently used feature.

@spytheman
Copy link
Member

We can also extend -d ident to support -d ident=value, then it can work as before for the first case (except that $compile_value(ident, false) will be true).

For the second case, it can have the new behavior, that is described here for -cv ident=value.

That way, there will be no need for a deprecation, it will continue to work, while having more functionality.

@felipensp
Copy link
Member

We can also extend -d ident to support -d ident=value, then it can work as before for the first case (except that $compile_value(ident, false) will be true).

For the second case, it can have the new behavior, that is described here for -cv ident=value.

That way, there will be no need for a deprecation, it will continue to work, while having more functionality.

+1 on keeping -d parameter.

@spytheman
Copy link
Member

gcc/clang's -D option also has 2 forms:
-Dname
-Dname=value
so there is an existing precedent for that.

@spytheman
Copy link
Member

It seems to work, but I have to do more tests:

#0 15:56:47 ^ comptime/key-value /v/oo>./v -d dynamic_boehm -keepc -showcc examples/hello_world.v
> C compiler cmd: '/space/v/oo/thirdparty/tcc/tcc.exe' '@/tmp/v_1000/hello_world.tmp.c.rsp'
> C compiler response file "/tmp/v_1000/hello_world.tmp.c.rsp":
  -fwrapv -o "/space/v/oo/examples/hello_world" -D GC_THREADS=1 -I "/usr/local/include" -L "/usr/local/lib" "/tmp/v_1000/hello_world.tmp.c" -std=gnu99 -D_DEFAULT_SOURCE -bt25 -lgc -lpthread -ldl
#0 15:56:52 ^ comptime/key-value /v/oo>./v -keepc -showcc examples/hello_world.v
> C compiler cmd: '/space/v/oo/thirdparty/tcc/tcc.exe' '@/tmp/v_1000/hello_world.tmp.c.rsp'
> C compiler response file "/tmp/v_1000/hello_world.tmp.c.rsp":
  -fwrapv -o "/space/v/oo/examples/hello_world" -D GC_BUILTIN_ATOMIC=1 -D GC_THREADS=1 -I "/space/v/oo/thirdparty/libgc/include" "/tmp/v_1000/hello_world.tmp.c" -std=gnu99 -D_DEFAULT_SOURCE -bt25 "/space/v/oo/thirdparty/tcc/lib/libgc.a" -ldl -lpthread
#0 15:56:59 ^ comptime/key-value /v/oo>
#0 15:57:00 ^ comptime/key-value /v/oo>
#0 15:57:00 ^ comptime/key-value /v/oo>./v -d my_f64=2.0 -d my_int=3 -d my_string='a four' -d my_bool -d my_char=g run ./vlib/v/gen/c/testdata/use_flag_comptime_values.vv
2.0
3
a four
true
g
#0 15:57:04 ^ comptime/key-value /v/oo>

@larpon
Copy link
Contributor Author

larpon commented Jun 17, 2024

I'm ok with keeping -d but then the name $compile_value will be less fitting IMO. Then maybe $compile_define would be a better name - or simply $define, but that may give too many C associations?

@spytheman
Copy link
Member

The PR is already updated to allow for -d name=value, and if everyone agrees, we can remove the code handling -cv/-compile-value in v.pref.

@spytheman
Copy link
Member

So, $if would remain the same? Or would it be different?

The syntax of $if is not affected at all by this PR (and it would be a breaking change if it did). It is still $if name ? {.

The new $compile_value('name', default) syntax allows you to get the actual passed value, which was not possible before.

I think, that it would be too verbose to be used as $if $compile_value('say_hi', false) {, even when that is possible (it currently is not evaluated in that case, which is a bug):

$if $compile_value('say_hi', false) {
        println('say_hi is true')
} $else {
        println('say_hi is false')
}

always prints say_hi is true, even for -d say_hi=false .

@felipensp
Copy link
Member

So, $if would remain the same? Or would it be different?

The syntax of $if is not affected at all by this PR (and it would be a breaking change if it did). It is still $if name ? {.

The new $compile_value('name', default) syntax allows you to get the actual passed value, which was not possible before.

I think, that it would be too verbose to be used as $if $compile_value('say_hi', false) {, even when that is possible (it currently is not evaluated in that case, which is a bug):

$if $compile_value('say_hi', false) {
        println('say_hi is true')
} $else {
        println('say_hi is false')
}

always prints say_hi is true, even for -d say_hi=false .

yes, $compile_value() is too verbose on if conditions. We just need to document about using -d to handling conditional code and accessing its values. How retrive value and how use it on conditions.

@larpon
Copy link
Contributor Author

larpon commented Jun 17, 2024

I'm ok with -d ident=value and like any of $d $define and $compile_value. $d is very nice since it matches -d.

Maybe it will be possible to do:
$if name == value ? {...}
For comptime logic?

@larpon
Copy link
Contributor Author

larpon commented Jun 18, 2024

Limited HashStmt support is added in e1be3eb. Currently we only support string values like with $env().

The whole # comptime setup could probably benefit from some proper refactoring at some point.

But now this works similar to $env() in #flag, #include etc.:

#flag -I $d('my_flag','flag_value')/xyz
#include "$d('my_include','/usr/include')/stdio.h"

@larpon larpon changed the title comptime: support -compile-value key=value and var := $compile_value/2 comptime: support -d ident=value and var := $d/2 Jun 18, 2024
@larpon
Copy link
Contributor Author

larpon commented Jun 18, 2024

Resolved conflict with master, hence the merge with master

doc/docs.md Outdated Show resolved Hide resolved
@spytheman spytheman changed the title comptime: support -d ident=value and var := $d/2 comptime: support -d ident=value and var := $d('ident', 0) Jun 18, 2024
vlib/v/util/util.v Outdated Show resolved Hide resolved
Co-authored-by: larpon <768942+larpon@users.noreply.github.com>
@spytheman spytheman merged commit 78b77b9 into vlang:master Jun 19, 2024
76 checks passed
@larpon larpon deleted the comptime/key-value branch June 19, 2024 08:02
raw-bin pushed a commit to raw-bin/v that referenced this pull request Jul 2, 2024
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.

7 participants