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

feature to use core::error::Error instead of std::error::Error #154

Merged
merged 5 commits into from
Oct 24, 2024
Merged

feature to use core::error::Error instead of std::error::Error #154

merged 5 commits into from
Oct 24, 2024

Conversation

ultimaweapon
Copy link
Contributor

@ultimaweapon ultimaweapon commented Oct 11, 2024

Description

Not sure if the validation outputs are required because it is a small change.

Closes #153.

API Stability

  • This PR does not require a breaking API change

Checklist

  • Documentation
    • Ensured any public-facing rustdoc formatting looks good (via cargo doc)
    • (if appropriate) Added feature to "Debugging Features" in README.md

@daniel5151
Copy link
Owner

Good start!

There are two other change you'll need to make before we can land this:

  • Also update GdbStubError to impl core::error::Error instead of std::error::Error when this feature is enabled. Should be a similarly easy fix.
  • Add a brief description of this new feature-flag to the docs in both README.md, as well as the doc-comment in lib.rs

And yeah, feel free to delete the Validation section from the template - no need for a PR like this.

@daniel5151
Copy link
Owner

And actually, now that I think about it - do we actually need a core::error::Error bound on the type Error?

I believe the downstream Debug + Display bounds on the various impl blocks should be giving users the flexibility to choose if they want to have their error type impl those various traits, without strictly requiring them to.

And thanks to the lack of type-erasure in gdbstub's API - if your error type implements core::error::Error, you'll still be able to leverage that implementation by using GdbStubError::into_target_error, which returns the concrete type you specified.

@ultimaweapon
Copy link
Contributor Author

And actually, now that I think about it - do we actually need a core::error::Error bound on the type Error?

I believe the downstream Debug + Display bounds on the various impl blocks should be giving users the flexibility to choose if they want to have their error type impl those various traits, without strictly requiring them to.

And thanks to the lack of type-erasure in gdbstub's API - if your error type implements core::error::Error, you'll still be able to leverage that implementation by using GdbStubError::into_target_error, which returns the concrete type you specified.

You're right. I'll revert this changes and convert std::error::Error to core::error::Error instead.

@ultimaweapon ultimaweapon marked this pull request as draft October 12, 2024 18:04
@ultimaweapon ultimaweapon changed the title add core::error::Error as a bound on Target::Error replace std::error::Error with core::error::Error Oct 12, 2024
@ultimaweapon
Copy link
Contributor Author

@daniel5151 do you have any preference how to choose between std and core version? I'm thinking to create a type alias but not sure if you would like to go this way. If yes let me know its name you like.

@daniel5151
Copy link
Owner

Apologies for the delay - we just open-sourced https://github.com/microsoft/OpenVMM at $work, so my attention has been elsewhere the past few days 😅

I think a type-alias based approach is good? i.e: having a single impl CoreError for Foo block, where a cfg block determines what type backs the type CoreError?

@daniel5151
Copy link
Owner

This approach looks good 👌

Two things before we can merge:

  1. Address the rustfmt related style issue
  2. Like I mentioned in feature to use core::error::Error instead of std::error::Error #154 (comment), make sure to update README.md and the doc-comment in lib.rs to mention this new feature.

@daniel5151 daniel5151 marked this pull request as ready for review October 24, 2024 15:51
@daniel5151
Copy link
Owner

Awesome. Thanks again for sending in the PR! I'll merge this in, and cut a new gdbstub release with this new feature.

@daniel5151 daniel5151 changed the title replace std::error::Error with core::error::Error feature to use core::error::Error instead of std::error::Error Oct 24, 2024
@daniel5151 daniel5151 merged commit f5354b6 into daniel5151:master Oct 24, 2024
2 checks passed
@ultimaweapon ultimaweapon deleted the error branch October 25, 2024 03:45
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.

Supports core::error::Error for Rust 1.81+ as a bound on gdbstub::target::Target::Error
2 participants