Skip to content

shared: switch to structured error handling#454

Merged
BurntSushi merged 6 commits intomasterfrom
ag/more-error-refactoring
Dec 20, 2025
Merged

shared: switch to structured error handling#454
BurntSushi merged 6 commits intomasterfrom
ag/more-error-refactoring

Conversation

@BurntSushi
Copy link
Copy Markdown
Owner

This completes the work started in #453 by switching everything in
jiff::shared to structured error handling. This module is somewhat
special in that it is "shared" with jiff-static.

As a result, jiff::shared had its own special bare bones error
type that didn't build a dynamic chain internally (which is what
jiff::Error does, similar to anyhow::Error). It has now been
replaced with structured errors entirely and no dynamic chains. I did
this because I didn't feel like rebuilding the chaining functionality
(or moving it into shared), and because it didn't seem that onerous
of a task.

But doing it this way did make it clear to me that the chaining in
jiff::Error is letting us avoid a lot of tedium. Without the dynamic
chaining, you wind up needing to define many more error types in order
to preserve decent contextualized error messages. It's quite annoying,
but doable at a small scale.

This does shrink LLVM lines in my Biff benchmark even more. Before this
PR, we were at 756,177 lines. After this PR, we are at 753,960. Nice!

In terms of compile times, it seems there isn't much of a change. Before
this PR:

Benchmark 1: cargo b
  Time (mean ± σ):      4.542 s ±  0.017 s    [User: 15.310 s, System: 2.128 s]
  Range (min … max):    4.518 s …  4.570 s    10 runs

Benchmark 2: cargo b -r
  Time (mean ± σ):      7.708 s ±  0.051 s    [User: 65.504 s, System: 2.580 s]
  Range (min … max):    7.633 s …  7.765 s    10 runs

And after:

Benchmark 1: cargo b
  Time (mean ± σ):      4.613 s ±  0.014 s    [User: 15.509 s, System: 2.116 s]
  Range (min … max):    4.595 s …  4.636 s    10 runs

Benchmark 2: cargo b -r
  Time (mean ± σ):      7.767 s ±  0.066 s    [User: 65.627 s, System: 2.588 s]
  Range (min … max):    7.689 s …  7.862 s    10 runs

Which is somewhat disappointing.

@BurntSushi BurntSushi force-pushed the ag/more-error-refactoring branch 2 times, most recently from c582184 to ed682ab Compare December 20, 2025 19:20
For this one, I defined the error type inline with it. This just felt
right and it does stay reasonably encapsulated.

I'll probably swing back and do the same for the time range error type?
@BurntSushi BurntSushi force-pushed the ag/more-error-refactoring branch from ed682ab to 0baf046 Compare December 20, 2025 19:31
I guess parsing a POSIX time zone doesn't actually require `alloc` any
more. That's cool.
I believe this reduces code size.
This follows the pattern I used for TZif and POSIX time zone parsing.
@BurntSushi BurntSushi force-pushed the ag/more-error-refactoring branch from 0baf046 to 70e2160 Compare December 20, 2025 19:33
@BurntSushi BurntSushi merged commit 831d3ef into master Dec 20, 2025
40 checks passed
@BurntSushi BurntSushi deleted the ag/more-error-refactoring branch December 20, 2025 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant