cpal icon indicating copy to clipboard operation
cpal copied to clipboard

Remove thiserror and replace it with handwritten Error impls

Open prokopyl opened this issue 3 years ago • 1 comments

This PR removes the thiserror dependency, and replaces it with handwritten Error implementations.

The goal of this PR is to remove some dependencies and to shorten compile times for downstream crates, to which thiserror contributes a fair amount since it's a proc-macro. On my computer this brings the cold release build time down from 14s to 9s!

While removing thiserror does add back some boilerplate that is annoying to write, it is still quite easy to read through, and it is also isolated in the error module where it doesn't impact any logic code. I think the maintainability impact is minimal, while the compile times and dependency bloat improvements are quite solid. :slightly_smiling_face:

prokopyl avatar Jul 15 '22 20:07 prokopyl

So we went from manual errors to failure in 1275db805bf6920e80e293254c1efe742c218061, and then to thiserror in d9d4a906c919ed3333a128de797cedad017b442d. I'm supporting this change but we need to make sure that there is no back and forth about this. One could say that back when the switch to failure was made, description was still not deprecated. But idk.

est31 avatar Jul 15 '22 21:07 est31

Yeah let's do this. Can you rebase this PR?

est31 avatar Oct 18 '22 14:10 est31

the compile times and dependency bloat improvements are quite solid.

Can you quantify this?

Ralith avatar Oct 18 '22 20:10 Ralith

Yeah let's do this. Can you rebase this PR?

Yes, apologies for the delay. The PR is now rebased. :slightly_smiling_face:

the compile times and dependency bloat improvements are quite solid.

Can you quantify this?

Yes, although this not a very accurate measurement, on my 8-core machine (4750U) this drops compile times from 14s to around 9s.

In terms of dependencies, here is the list of dependencies that are removed with this change (extracted from cargo-tree):

└── thiserror v1.0.31
    └── thiserror-impl v1.0.31 (proc-macro)
        ├── proc-macro2 v1.0.51
        │   └── unicode-ident v1.0.1
        ├── quote v1.0.20
        │   └── proc-macro2 v1.0.51 (*)
        └── syn v1.0.98
            ├── proc-macro2 v1.0.51 (*)
            ├── quote v1.0.20 (*)
            └── unicode-ident v1.0.1

prokopyl avatar Feb 25 '23 19:02 prokopyl