Hi Michal,
Thanks for the patch! Some notes below...
On Sun, Jul 14, 2024 at 6:02 PM Michal Rostecki vadorovsky@gmail.com wrote:
`CStr` became a part of `core` library in Rust 1.75, therefore there is no need to keep the custom implementation.
It would depend on the differences, right? i.e. for a reader, is this meant to imply there is no meaningful difference in what you point out below?
- It does not implement `Display` (but implements `Debug`).
One question that comes up when reading this is: are we losing `Display`'s output form?
Also, for clarity, please mention if there is a difference in the output of the `Debug` ones.
- Otherwise, having such a method is not really desirable. `CStr` is a reference type or `str` are usually not supposed to be modified.
The sentence seems to be cut, and it should probably try to explain better why it is undesirable, i.e. if it is needed for something like `DerefMut`, then it seems better to have a method.
static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($condition));
static CONDITION: &'static core::ffi::CStr = unsafe {core::ffi::CStr::from_bytes_with_nul_unchecked(core::concat!(stringify!($condition), "\0").as_bytes())};
This looks worse after the change and requires `unsafe`. Can we do something to improve it?
// SAFETY: Casting to CStr is safe because its internal representation// is a [u8] too.unsafe { &mut *(self.buf.as_mut_slice() as *mut [u8] as *mut CStr) }
I see Björn commented on this already -- `CStr`'s layout is not guaranteed (and is a `[c_char]` instead).
Also, the casting is not what is unsafe, so perhaps it may be clearer to reword the comment.
In addition, please format comments as Markdown.
-//! work <- new_work!("MyStruct::work"), +//! work <- new_work!(c"MyStruct::work"),
I agree as well that it may make sense to simplify the callers as much as possible, unless there is a need to have that flexibility.
Cheers, Miguel