softdevteam / yksom

Other
8 stars 6 forks source link

Homogenise as_isize's return types #192

Closed ltratt closed 3 years ago

ltratt commented 4 years ago

The main objective of this PR is to make Val::as_isize return Result<...>.

This not only brings it in line with as_usize, but means that if the user makes a dynamic type error that passes a non-int to code that calls as_isize, they'll get a backtrace rather than an internal compiler error!

@jacob-hughes It might be interesting for you (and me!) to do a Krun run before and after this PR? The first commit (probably) reduces overhead while the second commit might introduce some overhead. [The second commit is arguably a correctness fit, so I wouldn't reject it for performance reasons; but it might still be interesting to know what the overall effect is!]

jacob-hughes commented 4 years ago

LGTM, I'll get back to you with the Krun results before we merge it.

ltratt commented 4 years ago

Thanks!

jacob-hughes commented 4 years ago

As per our benchmarking observations in Krun, it seems that this PR has little to no impact on performance. I'm happy with the code changes. Are you happy for me to merge this?

ltratt commented 4 years ago

Works for me!

jacob-hughes commented 3 years ago

bors try

bors[bot] commented 3 years ago

try

Build succeeded:

jacob-hughes commented 3 years ago

Not sure why I didn't r+ this one straight away...

bors r+

bors[bot] commented 3 years ago

Timed out.

jacob-hughes commented 3 years ago

bors r+

bors[bot] commented 3 years ago

Build succeeded: