Of these commits 3d4d4c0 is the most interesting.

As is, only the bare minimum necessary to change for into iter was done. It isnt faster as is, but the following could be done to optimize :

1. since there are no outside side effects or dependency between elements (i think) rayon could be used.
2. filter_map is used in the bare minimum way. this could be combined with filter_maps to replace unwraps() or replace if else.
3. General optimization of code could be done with regard to the current map(). for instance, I wonder if encoding[encoding.len() - 2] or if encoding.len() == 1 && encoding.first().unwrap() == "largesets" couldn't be changed? I think a closer look is necessary, and I didn't want to change something without knowing and break it.

c918f37 and 0b3ff3b need special attention.

Since the length + 1 was not necessary, i removed it and changed some code (length - 1) downstream.

This does affect the match range, and so i changed the range to match the original behavior, but it looks strange now (0~510 instead of 512?)

other than that it is mostly sideeffectless changes, except 7d65129 which changed exit behavior.

If the original exit code 1 is strictly necessary there are other ways to do recover the behavior as well.

Being more strict with cargo clippy and cargo test looking more into cargo clippy -- -Wclippy::pedantic

As said before, pedantic is just that. Pedantic. There are many false positives and we don't need to fix all of them. But it still would be valuable to understand why they are false positives and document them if necessary.

If mess_difference <0.01 we see if coherence_difference > 0.02 and return partialord based on that. If not, we try to use multibyte usage difference if it is big enough.

comparing with multibyte_usage_a.abs() > f32::epsilon is idiomatic and includes when the value is 0.0 or some value very close to it.

However, it does not change the final accuracy at all.

Describe the bug This part of the code seems to be unreachable

The outer if ensures that cohrerence_difference is not 0.0. So that particular if statement should never be reached.

To Reproduce I don't know the intended case for these statements so I cannot reproduce the "negative case"

Expected behavior maybe the outer if needs to be different. In the original python code, chaos_difference == 0.0 and self.coherence == other.coherence this comparison is used.

Strips should_strip_sig_or_bom some blackmagic boolean logic 4899bf3

I doubled checked this with pen and paper, and tests all pass But still it would be a good idea to get another set of eyes here.

a85c5fe1dd249ef52c873e58d061fbe73c776a5c is worth looking at.

This would only be a problem when the file size is around usize::MAX which is around 2^64 bytes.

Using cargo clippy -- -Wclippy::pedantic to find some further low hanging fruit. The false positive rate is quite high, so I'm tempted to include some line allows like #![allow(cargo::range_plus_one)]

However I'm not sure you would agree that it is a good idea since the default cargo clippy does not complain. One compromise would be to have it enabled in a separate branch like pedantic or dev where we could use to merge recent fixes to look through the course in a strict(pedantic), yet context aware(we know some are false positives) way.

I'll be very honest here. most of these changes are superficial.

I hoped to isolate atleast some parts of the codes into smaller functions but it seems when we include the debug and trace that have long reaching dependencies, it is not easy.

One thing i'm curious about are all the fallbacks what are they used for and how are they different? It seems like they might be some artifact of previous implementation.

If they are necessary I'm wondering if some kind of enum couldn't be used instead?

Many of these changes revolve around unwrap.

There are more changes that could be done to improve performance, readability or correctness, but most would need atleast some kind of consideration as to the correct implementation.

If fixes like this is welcome and merged, I would open issues that would discuss potential changes and their considerations

Hi. I'm taking a look into your code and it seems bench does not work atm.

I am not yet familiar with the ins and outs of the code so I did what I felt was the minimum i could do.

Describe the bug In test_is_accentuated https://github.com/nickspring/charset-normalizer-rs/blob/cbe086f3df38a16815033309ae80455eef64cef4/src/tests/utils.rs#L28 This case is tested to see if it is false. “Ё” (U+0401) Cyrillic Capital Letter Io

The code being tested is here https://github.com/nickspring/charset-normalizer-rs/blob/cbe086f3df38a16815033309ae80455eef64cef4/src/utils.rs#L118

The problem here is that it is considered to have an diaeresis under current correct unicode decomposition rules (both NFKD and NFD) https://www.compart.com/en/unicode/U+0401 https://graphemica.com/%D0%81

(BTW this is different from almost exactly looking Unicode Character “Ë” (U+00CB) Latin Capital Letter E with Diaeresis

To Reproduce the icu4x crates can be used to decompose in rust. cargo add icu_normalizer I am actually trying to reimplement some parts of the code and that is how i discovered it.

pub(crate) fn is_accentuated(ch: char) -> bool {
    let nfd = icu_normalizer::DecomposingNormalizer::new_nfkd();
    let denormalized_string: String = nfd.normalize(ch.to_string().as_str());
    denormalized_string
        .chars()
        .any(|decomposed| match decomposed {
        '\u{0300}' // "WITH GRAVE"
        |'\u{0301}' // "WITH ACUTE"
        |'\u{0302}' // "WITH CIRCUMFLEX"
        |'\u{0303}' // "WITH TILDE"
        |'\u{0308}' //  "WITH DIAERESIS"
        |'\u{0327}' // "WITH CEDILLA"
         => true,
        _=> false,
    })
}

This new implementation directly tries to directly decompose the input character and try to see if unicode characters that indicate accents exist. Since “Ё” (U+0401) Cyrillic Capital Letter Io decomposes into Е Cyrillic Capital Letter Ie + Diaeresis '\u{0308}' the new code returns true, while the old code returns false (since diaeresis is not in the name)

Expected behavior “Ё” (U+0401) should return true.

Additional context Unicode standard is fast moving. A new standard every year and especially for CJK there are new codepoints added constantly. I think it is valuable to have an implementation that is up to date.

Btw I have almost finished my implementation using various components from https://github.com/unicode-org/icu4x It is a pure rust codebase worked on by both standard bodies and industry supporters such as google, so I feel like it would be a good library to rely upon.

Re : #3

This codebase has been ported from Python and a lot of the design patterns could be improved to be more idiomatic rust code. Such a move will make it easier to improve speed and maintainability, ensure correct operation from a rust point of view.

Some examples would be avoiding for loops, using matches instead of if chains etc.

Many require deeper consideration.

For example, this codebase has extensive use of f32. Unless using intrinsics, f64 are as fast as or faster than f32 in rust. Moreover, trying to cast to and back for f32 and f64 can harm performance and make it difficult to ensure correct code. For instance there are instances of exact compare between f32 and f64 variables, and this is very unlikely to operate in the intended way. If it is intended, it would be valuable to have documentation regarding that, suppressing relevant lints as well. However, if there is a need to maintain ABI compatibility or follow a specification it might be inevitable. Also, on-disk size could be a consideration. In summary f32 vs f64 handling could serve as both idiomatic code and speed but only if done right.

I will try to prepare some PRs that change some things. Despite my best efforts, I am sure that many of my changes or views might be based on a flawed understanding of the code, so feel free to explain why things were done the way they were. In such cases I will help with documentation.

As per our discussion in #2 for speed improvements the following has been suggested

• calc coherence & mess in threads
• or calc mess for plugins in threads (or some async?)
• or something other...

The paths I had in mind was these:

• Related to threads idea : use Rayon
  • Replace HashMap with concurrent DashMap (Current std HashMap implements rayon so not strictly necessary, but might be useful to look into regardless)
• Use replace hashing algorithm used in HashMap with FxHash, AHash, HighwayHash
  • aHash implemented #14
• ~Replace sort() with sort_unstable()~ #6
• Identfiy preallocation opportunities. For instance, replace Vec::new() with Vec::with_capacity()

Many of these are low hanging fruit and related to refactoring the code to idiomatic Rust code. For example, there are many for loops in this code. Iterator based code is more idiomatic, easier to improve with rayon, and interact better with allocation. (pushing items from within a for loop can cause multiple allocs and copies, while collecting an iterator can allow fewer allocations.)