cloudflare / boring

BoringSSL bindings for the Rust programming language.
367 stars 114 forks source link

Impl From for SslVersion #248

Closed rushilmehra closed 4 months ago

kornelski commented 4 months ago

You could implement try_from as an inherent method (not a trait), and change From impl to use ExtensionType::try_from(x).unwrap() to make it sound, or if panicking could be too disruptive, then try_from(x).unwrap_or(some_default).

rushilmehra commented 4 months ago

My rationale is that if we went with the panic route, I consider that a breaking change, so at that point we should just switch to TryFrom directly. The issue with providing a default is it could lead to unexpected behavior. If we take SslVersion as an example, and you were able to construct one that is "invalid", the worst that would happen is any API like set_min_proto_version would just fail because the underlying c++ code sanity checks the incoming u16. If we provide a default, then application code that accidentally creates an SslVersion with a bogus value will silently configure some default version.