Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove generics from Error by making fragment a String #642

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

RCasatta
Copy link
Contributor

This cause a reduction of IR lines emitted by the compiler of about 2% (using cargo llvm-lines), so this should improve compile times.

I've seen using the parse example to check the produced binary size, but it's a small snippet I am not sure it calls a representative portion of the API surface.

From a library perspective this is equivalent since the field is used only to be converted to string.

In theory externally someone could use the field in the error to do things so it comes at a cost.

@RCasatta RCasatta changed the title Remove generics from Error by turing fragment into String Remove generics from Error by making fragment a String Feb 26, 2024
@apoelstra
Copy link
Member

When no-std is on you need to explicitly import alloc::string::String I believe. This currently does not compile without std.

But concept ACK doing this. It seems like an improvement to me. Eventually we should go over all the error types like we have been doing with rust-bitcoin.

@tcharding
Copy link
Member

One day I'd love to get time (ie months) to focus on miniscript.

@RCasatta
Copy link
Contributor Author

no-std fixed

@apoelstra
Copy link
Member

I fixed CI in #645. I'd like to get that in and rebase this because it's the kind of thing that we want CI to be passing for.

@RCasatta
Copy link
Contributor Author

rebased to take #645

@apoelstra
Copy link
Member

Looks like you need to run cargo fmt.

@RCasatta
Copy link
Contributor Author

RCasatta commented Feb 29, 2024

Looks like you need to run cargo fmt.

done

It's still broken on nightly which seems to have alloc::string::String automatically imported, I am not sure how to fix

@apoelstra
Copy link
Member

When std is on then String, ToString etc are always imported. It's just that nightly warns about this. There are half a dozen open issues on rustc related to this new lint triggering all the time in counterproductive ways.

Meanwhile to deal with it you just need to make sure your imports are feature-gated on nostd.

This cause a reduction of IR lines emitted by the compiler of about 2%
(using cargo llvm-lines), so this should improve compile times.

I've seen using the parse example to check the produced binary size, but it's
a small snippet I am not sure it calls a representative portion of the API
surface.

From a library perspective this is equivalent since the field is used only
to be converted to string.

In theory externally someone could use the field in the error to do things so
it comes at a cost.
@RCasatta
Copy link
Contributor Author

RCasatta commented Mar 1, 2024

ah right the same you did here

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0afd832

@apoelstra
Copy link
Member

cc @sanket1729 can you concept-ACK this?

@apoelstra
Copy link
Member

On further thought I'm just going to merge this. I have some bigger ideas about how to handle errors in this crate, and getting rid of these generics will make future refactoring much easier.

@apoelstra apoelstra merged commit 02504cb into rust-bitcoin:master Mar 4, 2024
16 checks passed
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.

3 participants