schungx / rhai

Rhai - An embedded scripting language for Rust [dev repo, may regularly force-push, pull from https://github.com/rhaiscript/rhai for a stable build]
https://github.com/rhaiscript/rhai
Apache License 2.0
9 stars 3 forks source link

Add combine_with_exported_module and a type cast fix for constants. #56

Closed schungx closed 4 years ago

schungx commented 4 years ago

@jhwgh1968 I'm beginning to study the proc macros code. This is a small attempt to add combine_with_exported_module which flatten-generate module functions into an existing module.

I also fix a small problem with types when processing constants in modules. The expression was not originally guaranteed to be of the same type as the constant definition. I added a type cast to make sure that it is the same.

schungx commented 4 years ago

I can see that I need to fix the tests in src/test/module.rs to match the actually generated code.

I'll do that gradually.

jhwgh1968 commented 4 years ago

Looking at the PR, I would say the new function does no harm... except there are a couple changes to the unit tests (under src/test) that surprise me. I would like to understand those.

Since more test changes are coming, I will wait until the PR passes CI before doing a detailed review. That context will help.

schungx commented 4 years ago

Looking at the PR, I would say the new function does no harm... except there are a couple changes to the unit tests (under src/test) that surprise me. I would like to understand those.

I think the main one is that I added an extra parameter to constants, keeping its type.

mod some_mod {
    const foo: INT = 42;        // 42 is volatile
    const foo2: INT = 42 as INT;     // 42 is always INT
}

In the case above, 42 will be cast to the system default integer type, not INT as specified. That's because, internally, the macro creates a set_var call with just the expression in it, throwing away the type.

m.set_var("foo", 42);

I changed it to cast it to the right type:

m.set_var("foo", (42) as INT);
schungx commented 4 years ago

@jhwgh1968 Yay! Tests passed!

Basically, I added rhai_generate_into_module which registers functions into an existing module. Then rhai_generate is modified to simply call that. Unfortunately that means src/test/module.rs needs to be extensively modified to match the new generated tokens.

jhwgh1968 commented 4 years ago

Thanks to the extra permissions you've given me, I can simply clean up your branch, rather than trying to explain another idea I had on how to do it! :sparkles:

I will review that version once I push it.

schungx commented 4 years ago

If possible, maybe clean up the commit history? I screwed up now there are many duplicated commits... Don't know how to fix it...

schungx commented 4 years ago

I am still thinking there is a better way to handle constants than duplicating them all, but first, some other comments.

Maybe keep the constant definition, but then use it in the set_var call instead of duplicating the code with a cast?

schungx commented 4 years ago

I am still thinking there is a better way to handle constants than duplicating them all, but first, some other comments.

Changed set_var(#const_name, (#const_expr) as #const_type) to set_var(#const_name, #const_value) where const_value is Ident(const_name).

New commits:

1) Remove Box from const type. 2) Removed 50-character printout for token stream asserts.

jhwgh1968 commented 4 years ago

I have reviewed again, and cleaned up the extra merge commit. LGTM. Once CI passes, I will approve.

jhwgh1968 commented 4 years ago

As a side note, I just found this: Git rebase in Visual Studio Code?

They say you can avoid the merge commits with a 3rd party extension which appears to be free.

schungx commented 4 years ago

Yes, I have been using git pull --rebase as you've taught. However, it seems like the commit history lists keeps going longer on this one...

It must have been Visual Studio Code that is doing this, but I"m not sure. It could also be something that I've done wrong...

Maybe we fix this PR and merge it, then I'll scrap plugins_dev and open a new, clean one...

jhwgh1968 commented 4 years ago

Whoa, hang on, you just added another merge commit! I was actually hoping you would wait to merge it before you did anything else.

Should I fix it again? Or are you trying to add something else?

EDIT: your comment was slow to render. I will fix it.

EDIT 2: based on that Stack Overflow thread I linked, I think it is VS Code at fault here.

schungx commented 4 years ago

Yup. I just pressed "Sync" and it added a merge commit, for some weird reason. :-(

Please give this a fix. I'll actually delete my local branch and pull from here.

jhwgh1968 commented 4 years ago

Alright, it's fixed!

schungx commented 4 years ago

Thanks! I deleted my local branch and then git checkout -b plugins_dev origin/plugins_dev and it seems to be working now...

schungx commented 4 years ago

Hhhmmm... for some reason, three no-std tests remain pending for a long time.

I'm just merging this...