Rust::com Derive macro for CommData#156
Rust::com Derive macro for CommData#156bharatGoswami8 wants to merge 1 commit intoeclipse-score:mainfrom
Conversation
bharatGoswami8
requested review from
LittleHuba,
castler and
hoe-jo
as code owners
bharatGoswami8
force-pushed
the
rust_com_api_derive_macro_commData
branch
from
719a3c7 to
7524795
Compare
* Implemented derive macro for CommData trait impl
bharatGoswami8
force-pushed
the
rust_com_api_derive_macro_commData
branch
from
7524795 to
14a7f51
Compare
pawelrutkaq
left a comment
pawelrutkaq
left a comment
There was a problem hiding this comment.
LGTM, one issues found
| #[derive(Debug, Reloc)] | ||
| #[derive(Debug, CommData)] | ||
| #[repr(C)] | ||
| // No explicit ID provided, so it will be auto-generated as "com_api_gen::Exhaust" |
There was a problem hiding this comment.
Rechecked an my bad -all good.
|
|
||
| /// Derive macro for the `CommData` trait. This macro automatically implements the `CommData` trait | ||
| /// for structs and C-like enums, ensuring that the type is marked with `#[repr(C)]`. | ||
| /// It internally also derives the `Reloc` trait for the type. |
There was a problem hiding this comment.
Wouldn't it be better if Reloc is a supertrait of CommData and the user needs to explicitly #[derive] this as well? The same is true for e.g. Copy, which requirees Clone to be derived as well. It would make this aspect explicit without adding a lot of extra typing required.
There was a problem hiding this comment.
You're right. Auto‑implementing Reloc cuts down on boilerplate, but it hides the fact that the trait is needed. Just like how Copy requires Clone in Rust, CommData should explicitly derive Reloc. This makes the dependency clear in the code, and helps users understand what’s going on. The small amount of extra typing is worth the clarity.
| /// It internally also derives the `Reloc` trait for the type. | ||
| /// | ||
| /// # Important | ||
| /// Users must NOT implement the `CommData` trait manually. Always use this derive macro instead. |
There was a problem hiding this comment.
Why not? What constraints exist here and could they be enforced somehow so that the user could also implement this manually?
There was a problem hiding this comment.
Users shouldn’t implement this manually because their type must have #[repr(C)], and only the macro can verify that.
I tried preventing manual implementations of CommData with a private type and a sealed trait, but that added extra exported items in generated user scope, which still let users implement the trait. So my option is to explain this clearly in the concept crate, with warnings telling users not to implement CommData or Reloc themselves.
There was a problem hiding this comment.
Possible solution -
by making CommData trait unsafe and detail documentation, we can enforce user to use macro of CommData and if manually implementation then also user need to make sure it is implemented correctly.
There was a problem hiding this comment.
If #[repr(C)] is required then marking this trait unsafe is a requirement because to my knowledge you cannot enforce this using a language construct.
There was a problem hiding this comment.
for me unsafe is a good tradeoff. At the end, in 99% user shall still not do it manually and the advice in doc shall be left to use derive.
3 participants
#155