-
Notifications
You must be signed in to change notification settings - Fork 3
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
New mess detection algorithm #31
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am looking into other bitflag alternatives or ways to collapse the if chains
(while maintaining correctness or performance)
But I don't see why it shouldn't be merged as is. we can always fix later
Cargo.lock
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be a good idea to add Cargo.lock into .gitignore?
According to best practices, it is up to the project.
Ignoring it helps with development churn. But divergent crates could harm reproducibility.
Keeping it helps with reproducible builds, but not alone. You require proper reproducibility toolchain as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read about it and didn't decided what's better. But I guess Cargo.lock was there but at the moment it was incompatible with my system (I think it can be incompatible if we will build lib for different system and push Cargo.lock to the repo. However, I can try to add it again.
src/md.rs
Outdated
&& !is_hiragana(character) | ||
&& !is_thai(character); | ||
|
||
if !self.foreign_long_watch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be rewritten like this?
foreign_long_watch |= (!character.is(MessDetectorCharFlags::LATIN) || (character.is(MessDetectorCharFlags::ACCENTUATED))
&& !character.is(MessDetectorCharFlags::CJK)
&& !character.is(MessDetectorCharFlags::HANGUL)
&& !character.is(MessDetectorCharFlags::KATAKANA)
&& !character.is(MessDetectorCharFlags::HIRAGANA)
&& !character.is(MessDetectorCharFlags::THAI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sure. I didn't think about idiomatic code at all - my plan was to do it and see new performance and after that moment rewrite in more idiomatic way with performance controlling :)
As for idiomatic vs speed, I think the biggest change here in the code is many of the chars and flags are owned by the If this gets merged I will
|
In progress