qsantos commented 1 year ago

Following https://github.com/servo/rust-url/pull/817#pullrequestreview-1325304510, it was raise that port.to_string().len() could be optimized to avoid allocating a string.

qsantos commented 1 year ago

Ah, the CI is targeting 1.51.0 but checked_ilog10 is from 1.67.0. Did you have something else in mind @klensy?

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 87.50% and no project coverage change

Comparison is base (74b8694) 82.73% compared to head (28c2d6c) 82.73%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #823 +/- ## ======================================= Coverage 82.73% 82.73% ======================================= Files 20 20 Lines 3330 3337 +7 ======================================= + Hits 2755 2761 +6 - Misses 575 576 +1 ``` | [Impacted Files](https://codecov.io/gh/servo/rust-url/pull/823?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo) | Coverage Δ | | |---|---|---| | [url/src/slicing.rs](https://codecov.io/gh/servo/rust-url/pull/823?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo#diff-dXJsL3NyYy9zbGljaW5nLnJz) | `91.30% <87.50%> (-0.64%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

klensy commented 1 year ago

It can be something like https://stackoverflow.com/a/69298721 where you simply multiply by 10 and compare.

qsantos commented 1 year ago

I have made it a fallback with some tests for edge cases. What do you think?

valenting commented 1 year ago

How about a simple match statement? https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b2fffe2e3cc3683b55cf4d0e0789795b

fn count_digits(n: u16) -> u8 {
    match n {
        0..=9 => 1,
        10..=99 => 2,
        100..=999 => 3,
        1000..=9999 => 4,
        10000..=65535 => 5,
    }
}

qsantos commented 1 year ago

They compile to the same binary! In fact, even for u32 or u64, the compiler just unrolls the loop. It does use a trick for u64 because there are no CMP instructions with an immediate 64 bit value on x86_64 (if I understand correctly).

In any case, your version looks much nicer, and it is much easier to convince oneself that it is correct, so I have pushed it.

servo / rust-url

Avoid string allocation to get length of port #823

Codecov Report