Skip to content

add Debug attr#70

Merged
danlehmann merged 4 commits intodanlehmann:mainfrom
us-irs:debug-impl
Mar 27, 2025
Merged

add Debug attr#70
danlehmann merged 4 commits intodanlehmann:mainfrom
us-irs:debug-impl

Conversation

@robamu
Copy link
Collaborator

@robamu robamu commented Mar 13, 2025

This adds a debug attribute, which generates a simple Debug implementation which forwards to the inner fields.
I also did a little bit of refactoring for the parsing of the attributes to be more syn like :) THis is based on the syn docs: https://docs.rs/syn/latest/syn/meta/fn.parser.html

I also added a small example. I think this is really useful for dev purposes, because I have a concrete example target I can cargo expand for bugfinding. It might also be useful for users.

Some other things I noticed:

  • What do you think about moving the docs to the lib.rs file? rustdoc is even able to compile check some of the examples.
    In the README, refer to the docs on docs.rs, or use a docs.rs badge :)
  • Why are the unit-tests in a separate crate? And If some error tests using trybuild, or a trybuild test infrastructure
    in general is added, can this be done just in the bitfield crate?

@danlehmann
Copy link
Owner

Thanks for your contribution. Will review and test and get back to you asap

@danlehmann danlehmann self-requested a review March 13, 2025 17:52
@danlehmann
Copy link
Owner

I merged a patch that will execute tests for PRs (like this). Would you mind rebasing to tip of tree?

@robamu
Copy link
Collaborator Author

robamu commented Mar 21, 2025

I can do that

@@ -1,3 +1,6 @@
use core::fmt;
Copy link
Owner

Choose a reason for hiding this comment

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

this causes a warning when building the tests

.collect();
debug_trait.append_all(quote! {
impl core::fmt::Debug for #struct_name {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
Copy link
Owner

Choose a reason for hiding this comment

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

this should be core::fmt::Formatter. This crate user might be no_std

Copy link
Owner

Choose a reason for hiding this comment

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

Same thing for the result

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, I think that might bring its own problems for users that are using std. I think we need a way to figure out whether to use std or core. No idea how we would do that though - we could use the non-qualified name I guess? But that would be annoying to use.

Copy link
Owner

@danlehmann danlehmann Mar 27, 2025

Choose a reason for hiding this comment

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

Nevermind what I said before, I just tried it out. Using ::core::fmt::Debug should always work, both in no_std and std projects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, good catch.. it might be a good idea to have some sort of test for no_std compatiblity. The easiest way is a dedicated no_std app which is checked by CI

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a no_std check

@danlehmann
Copy link
Owner

Thank you for going above and beyond. This is excellent!

@@ -0,0 +1,12 @@
#![no_std] #![no_main]

#[bitbybit::bitfield(u32)]
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add debug and default? Those both require using traits from std

Copy link
Owner

Choose a reason for hiding this comment

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

Oh you already did. I'll go ahead and merge this

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.

2 participants