-
Notifications
You must be signed in to change notification settings - Fork 1
Add #[bitmap_attr] macro - issue #8 #9
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
Add #[bitmap_attr] macro - issue #8 #9
Conversation
- Implements attribute macro that provides the same functionality as bitmap! - Both macros now generate identical code - Added comprehensive tests to ensure compatibility - Maintains all existing functionality while providing cleaner syntax option
- Implements feature requested in issue winstonallo#8 - Provides alternative syntax to bitmap! macro - Both macros generate identical code and functionality - Added comprehensive tests to verify compatibility - Updated README with usage examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @RabbitAlbatross, I reviewed your changes, once everything is addressed and the CI passes we should be good to go!
It seems like you have some LLM comments sprinkled across your PR, please remove all of them (example: https://github.com/winstonallo/bitmap/pull/9/files#r2393549520). The code is straightforward, and they do not add any value in my opinion.
Thanks a lot for your PR!
macros/src/lib.rs
Outdated
| fn convert_derive_to_bitmap_input(input: DeriveInput) -> parser::BitmapInput { | ||
| let name = input.ident; | ||
|
|
||
| // Extracting struct fields | ||
| let syn::Data::Struct(data_struct) = input.data else { | ||
| panic!("#[bitmap_attr] can only be used on structs"); | ||
| }; | ||
|
|
||
| let syn::Fields::Named(fields_named) = data_struct.fields else { | ||
| panic!("#[bitmap_attr] struct must have named fields"); | ||
| }; | ||
|
|
||
| // Converting each field to the format expected by the existing parser | ||
| let fields = fields_named.named.into_iter().map(|field| { | ||
| let field_name = field.ident.expect("Field must have a name"); | ||
| let field_type = field.ty; | ||
|
|
||
| // Extracting the size from the type (e.g., u1 -> 1, u7 -> 7) | ||
| let type_str = field_type.to_token_stream().to_string(); | ||
|
|
||
| if !type_str.starts_with("u") { | ||
| panic!("Field type must be unsigned integer like u1, u2, etc."); | ||
| } | ||
|
|
||
| let size: u8 = type_str[1..].parse().expect("Invalid bit width"); | ||
|
|
||
| if size == 0 || size > 128 { | ||
| panic!("Invalid size for {}, expected u{{1..128}}", type_str); | ||
| } | ||
|
|
||
| parser::FieldDef { | ||
| name: field_name, | ||
| size, | ||
| } | ||
| }).collect(); | ||
|
|
||
| parser::BitmapInput { | ||
| name, | ||
| fields, | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer it if you would solve this in a similar way as for the function-like macro, by defining a type and implementing the syn::Parse trait for it.
macros/src/lib.rs
Outdated
| let type_str = field_type.to_token_stream().to_string(); | ||
|
|
||
| if !type_str.starts_with("u") { | ||
| panic!("Field type must be unsigned integer like u1, u2, etc."); | ||
| } | ||
|
|
||
| let size: u8 = type_str[1..].parse().expect("Invalid bit width"); | ||
|
|
||
| if size == 0 || size > 128 { | ||
| panic!("Invalid size for {}, expected u{{1..128}}", type_str); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is needed in both bitmap! and #[bitmap_attr], can you please extract it to a function in case we have to change the parsing logic in the future?
|
|
||
| #[proc_macro_attribute] | ||
| pub fn bitmap_attr(_args: TokenStream, input: TokenStream) -> TokenStream { | ||
| let input = parse_macro_input!(input as DeriveInput); | ||
|
|
||
| // Converting the attribute macro input to the existing BitmapInput format | ||
| let bitmap_input = convert_derive_to_bitmap_input(input); | ||
|
|
||
| // Use the EXACT SAME expansion logic as the bitmap! macro | ||
| match generator::expand_bitmap(bitmap_input) { | ||
| Ok(tokens) => tokens.into(), | ||
| Err(err) => err.to_compile_error().into(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add documentation, it can be the same as for bitmap! (except for the examples of course).
macros/src/lib.rs
Outdated
| pub fn bitmap_attr(_args: TokenStream, input: TokenStream) -> TokenStream { | ||
| let input = parse_macro_input!(input as DeriveInput); | ||
|
|
||
| // Converting the attribute macro input to the existing BitmapInput format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove those LLM comments.
removing unnecessary llm comments
|
Requested changes completed |
@RabbitAlbatross The comments are still there, and this comment does not seem to have been addressed. https://github.com/winstonallo/bitmap/actions/runs/18161824100/job/51695838175?pr=9 |
|
Is it fine now? |
|
@RabbitAlbatross You still did not address this comment, and the CI is failing, even the tests you wrote. |
|
I will close this PR now, I do not think you are serious about this. |
|
Sorry I was busy but I guess it's fine. May you find someone better to help with the project. Best of luck in your future endeavors. |
|
@RabbitAlbatross For clarification, this is not because of how long it took you, I am thankful for contributions! I just do not have the feeling that you took this PR seriously, the code looks AI-generated and directly copied, you did not address half of my comments, and did not even run the |
|
I did clone it and write the code myself but when it was bugging while I was running tests locally I turned to AI for debugging. I later only made changes on the online gui after I made the PR because I thought a couple of changes might resolve it. I'm sorry for the sub-par contribution. I'm still a student and pretty new to Rust in general. You deserve someone actually competent for the project. Thank you for giving me time. |
|
Thanks for the explanation. It's okay to use AI for debugging, or to be new to Rust, but feel free to ask questions if you need guidance in the future. Also a proc-macro might not be the best way to get started with Rust, they can be quite confusing. |
This adds the attribute macro from issue #8.
Now you can do:
Instead of:
Both ways work the same and make the same struct with the same methods. The tests show they work identically.
Fixes #8