Heiss/magic-regexp

Is it necessary to use String for Text(String) variant

Opened this issue · 6 comments

alexpyattaev commented

Great idea to rid the world of regex=) Small performance question - when creating the match pattern, for Text it is necessary to allocate memory for every small bit of pattern, which is a lot of allocations. Is it possible for Text to take a string reference instead?

Heiss commented

Maybe not, but this was a first idea.

magic-regexp/src/core/type.rs

Line 22 in e2c45fa

Text(String),

If you know a way to store a reference in enums, it would be an easy fix.

Heiss commented

I implemented it now like you suggested. Was easy and gives some more insides about rust. :)

Takes now &str everywhere possible. Only in some cases it needs allocations and a dirty hack with Box::leak. But i think, this are the rare use cases, where it is okay. Maybe there could be more things to be done, when it comes to compile-time optimization. But this is right now far over my head. :)

Thank you for your input.

alexpyattaev commented

I am glad that you are improving your rust skills while also maintaining this! I'll look into the Box::leak trick that you are using now to check if it is sound, if not there are some mechanisms for string manipulation at compile time I can try. Stay tuned for a PR.

alexpyattaev commented

Ok just checked the not(Text("something")), and it would appear that while it indeed does not leak memory, it does not actually work (or I am missing what it is supposed to be doing). Can you clarify? Regex does not appear to have a notion of negative match on text that makes sense in general case. Thanks!

Heiss commented

Okay sad. Then i need another workaround and more tests to catch this case. :) Thank you for your testing.

Regex does not appear to have a notion of negative match on text that makes sense in general case

Nah. My intuition would say: The equivilant of Not from a Text would be: "I assume, that this text is not here". So it would make sense to me that there is an opposite of text. :)

Given example should clarify this.

let input = Not(Exactly(Text("foo"))).and(Exactly(Text("bar")));
let re = create_reg_exp(input).unwrap();

assert!(!re.is_match("foobar"));  // Attention: Exclamation mark!
assert!(re.is_match("barbar"));
assert!(re.is_match("fooShouldNotBeAProblemAsLongAsItIsNotInFrontOFbar"));

But this is a different statement as it is right now implemented.

If we confirm, that this would be the expected behaivour, then yes you are right, this does not work right now. :)

alexpyattaev commented