search

greyblake / whatlang-rs

Natural language detection library for Rust. Try demo online: https://whatlang.org/
https://whatlang.org/
MIT License
970 stars 109 forks source link

Implemented Lang::from_code() via procedural macros #7

Closed alekseyl1992 closed 7 years ago

alekseyl1992 commented 7 years ago

to_code() / from_code() implemenetation looked a bit weird to me and I wanted to get some expirience in Rust language programming so I've made this pull request :)

pattern matching operator in from_code() is now being generated via procedural macros docs: https://doc.rust-lang.org/book/procedural-macros.html

I wanted to make from_string() function universal, so EnumFromString could be applied to any enum that's why there is from_string() in EnumFromString trait and from_code() in concrete implemetation for Lang enum

I also wanted to move EnumFromString inside of the whatlang-derive library, but macro libraries can only export functions for now:

error: proc-macro crate types cannot export any items other than functions tagged with #[proc_macro_derive] currently

as for benchmarks:

before change:

running 2 tests
test bench_detect        ... bench:  29,613,380 ns/iter (+/- 307,791)
test bench_detect_script ... bench:     227,694 ns/iter (+/- 689)

after change:

running 2 tests
test bench_detect        ... bench:  29,496,420 ns/iter (+/- 387,359)
test bench_detect_script ... bench:     224,283 ns/iter (+/- 1,048)

it's almost just not changed

clarfonthey commented 7 years ago

Shouldn't these macros be exported to another crate? Not necessarily part of whatlang, but usable by any crate?

alekseyl1992 commented 7 years ago

It could be.. But I should figure out how to move the trait definition (EnumFromString) from client code to the library beforehand. For now each time one want to derive EnumFromString, one should define that trait in their crate :(

To rephrase: looking at this example: https://doc.rust-lang.org/book/procedural-macros.html is it possible to move definition of trait HelloWorld from hello-world crate to hello-world-derive without getting error: proc-macro crate types cannot export any items other than functions tagged with #[proc_macro_derive] currently error?

alekseyl1992 commented 7 years ago

Ok, I've figured out a way to get rid off the EnumFromString trait. Now EnumFromString macro genrates implemetations for FromStr and Display standard traits. It also provides get_variants() -> Vec function to get all possible enum variants as a vector.

I think soon that week I'll move that code to a separate crate and publish it on crates.io. I'll also split these function to separate macros, so end users can choose what they want.

alekseyl1992 commented 7 years ago

It turned out, that there is already exists a crate, which provides FromStr derivation: https://github.com/AndyShiue/derivation

So I replaced my code with this crate.

greyblake commented 7 years ago

@alekseyl1992 Hi, sorry for a late reply!

Thanks for the suggestion, I appreciate your effort. I agree, the way enums are managed enums is shitty, actually I use ruby script to generate that code. But I can not merge your PR because of the following reasons:

However, I admit enums are not done in a nice way..

alekseyl1992 commented 7 years ago

This PR adds a new external dependency (I am trying to keep the crate free of runtime deps)

There was hand-written implementation in the previous commit: https://github.com/greyblake/whatlang-rs/pull/7/commits/7181c0174a1fdf83ba782f2e45f07fe568cc4009

The whole idea of enums and that static code that is replaced in this PR is performance. Replacing it with dynamic code makes it 20%-25% slower as your benchmark shows.

I've replaced hardcoded match expressions with compile-time-generated ones. There is no extra runtime code.

And about benchmark in the first message: 29,613,380 ns/iter > 29,496,420 ns/iter 227,694 ns/iter > 224,283 ns/iter

so my version is a bit faster, than original one as for as I understand numbers in the braces (+/- something) - is a deviation between iterations am I wrong?