Various code improvements

[roderickvd]

This is a collection of code improvements:

1. upgrade deprecated std::u16::MAX constants
2. use FromStr for fallible &str conversions everywhere
3. upgrade legacy as_ref().map() calls into as_deref()
4. DRY up strings into constants
5. use Duration for time constants and functions

[Johannesd3]

FromStr is older than TryFrom, but it has the advantage that you can use str::parse to convert a string into this type. Using From + panic if conversion can fail would be much worse.

I also have the feeling that constants like ONE_SEC_AS_MILLIS wouldn't be necessary if Duration would be used more idiomatic.

[roderickvd]

FromStr is older than TryFrom, but it has the advantage that you can use str::parse to convert a string into this type. Using From + panic if conversion can fail would be much worse.

There's mixed use of FromStr and TryFrom in dev now. Would you rather move everything to FromStr?
Right now it all panics regardless, it'd be a separate PR to fail gently instead of calling expect.

I also have the feeling that constants like ONE_SEC_AS_MILLIS wouldn't be necessary if Duration would be used more idiomatic.

Good idea.

[roderickvd]

The nice Duration::SECOND and friends are only in Rust nightly unfortunately.

[JasonLG1979]

The nice Duration::SECOND and friends are only in Rust nightly unfortunately.

What's to prevent you from creating a const out of Duration::from_millis(1000) and just calling that ONE_SECOND or whatever?

[roderickvd]

That's the same as defining ONE_SEC_AS_MILLIS but without the syntactic sugar.

[JasonLG1979]

That's the same as defining ONE_SEC_AS_MILLIS but without the syntactic sugar.

If you're defining time in 1 sec chunks the _AS_MILLIS part of the name serves no purpose. When I think of idiomatic I think of how a person would speak. I wouldn't say "In one second of milliseconds I will do this."

[roderickvd]

I think I see what you guys mean, take a look at da1eded. It compiles but still needs testing.
Edit: I also changed a lot of f64 to f32 here, there is no point to having that precision just to calculate millisecond durations.

Also let me know about FromStr or TryFrom and I'll update if necessary.

[JasonLG1979]

I think I see what you guys mean, take a look at da1eded. It compiles but still needs testing.

I guess my nitpick is that if Duration::from_secs(TIMEOUT_SECONDS) is never going to yield a different result there's no point in calculating it over and over again. It should just be calculated once and used as a const. I may be totally wrong and misunderstanding what's going on though,lol!!!

[roderickvd]

I think I see what you guys mean, take a look at da1eded. It compiles but still needs testing.

I guess my nitpick is that if Duration::from_secs(TIMEOUT_SECONDS) is never going to yield a different result there's no point in calculating it over and over again. It should just be calculated once and used as a const. I may be totally wrong and misunderstanding what's going on though,lol!!!

In Rust, values from non-static functions cannot be assigned to a constant.

[JasonLG1979]

In Rust, values from non-static functions cannot be assigned to a constant.

Weird that something that would never change, the system's value for a sec, can't be be assigned to a constant?

[roderickvd]

It's being changed in Rust nightly but that won't be in librespot MSRV for a couple of years as long as we track Debian stable rustc.

[JasonLG1979]

It's being changed in Rust nightly but that won't be in librespot MSRV for a couple of years as long as we track Debian stable rustc.

Crusty old Debian stable,lol!!!

[Johannesd3]

I guess my nitpick is that if Duration::from_secs(TIMEOUT_SECONDS) is never going to yield a different result there's no point in calculating it over and over again. It should just be calculated once and used as a const. I may be totally wrong and misunderstanding what's going on though,lol!!!

"Calculating" would be the wrong word here. I'm pretty sure the compiler would optimize it in a way that hasn't more overhead than using a const value. But ...

It's being changed in Rust nightly but that won't be in librespot MSRV for a couple of years as long as we track Debian stable rustc.

I don't think that's true. Every const fn can be used to calculate the value of a const. And Duration::from_* are const for a long time.

Also let me know about FromStr or TryFrom and I'll update if necessary.

Honestly, I don't know. Maybe search for it in the internet, we're certainly not the only ones who wonder what's better.

[JasonLG1979]

"Calculating" would be the wrong word here. I'm pretty sure the compiler would optimize it in a way that hasn't more overhead than using a const value.

I'm not so sure about that? The compiler can't even optimize a "round to even" situation to not take twice the CPU,lol!!!

[JasonLG1979]

Plus if you could do a const it would look a Hell of a lot cleaner.

[Johannesd3]

Look: https://godbolt.org/z/E9GrbTox6. The compiler doesn't even generate code for the second function because it knows they're the same.

Plus if you could do a const it would look a Hell of a lot cleaner.

Agreed. But you can do.

[JasonLG1979]

Look: https://godbolt.org/z/E9GrbTox6. The compiler doesn't even generate code for the second function because it knows they're the same.

I was just poking fun. You know a lot more about it then I do,lol!!!

Agreed. But you can do.

Then we should do. By "we" I mean we in the royal sense, as in @roderickvd should do,lol!!!

[roderickvd]

Honestly I didn't even try, but now see that this works so I'll get on it:

use std::time::Duration;

const TIMEOUT: Duration = Duration::from_secs(1);

fn main() {
    println!("{:?}", TIMEOUT);
}

Also let me know about FromStr or TryFrom and I'll update if necessary.

Honestly, I don't know. Maybe search for it in the internet, we're certainly not the only ones who wonder what's better.

Man, huge can of worms. I don't particularly care either way, I just want to make them consistent. I'm fine with changing my proposal if anyone thinks that'd be better. Otherwise if no one has an opinion on this, might as well stick with it.

Look: https://godbolt.org/z/E9GrbTox6. The compiler doesn't even generate code for the second function because it knows they're the same.

Edit: handy website, learned something today.

[Johannesd3]

I would stick to FromStr as far as lifetimes allow it.

[roderickvd]

I would stick to FromStr as far as lifetimes allow it.

Here you go.

Plus if you could do a const it would look a Hell of a lot cleaner.

Thanks for the push into this direction. Using Duration everywhere does look a lot nicer.

[roderickvd]

If there are no further comments, or people who need more time, I plan on merging this evening. Will merge dev in and get the checks running. I successfully verified that preloading and dynamic normalisation are still working.

[Johannesd3]

You could write the comments for the constants above the constants and make rustdoc comments out of it

[roderickvd]

@Johannesd3 this what you had in mind?

[Johannesd3]

No, I meant those in audio/src/fetch/mod.rs

[roderickvd]

🤦‍♂️ this should be it then. Intend to squash merge when CI passes.