Re: [PATCH] rust: str: Use `core::CStr`, remove the custom `CStr` implementation

[Miguel Ojeda]

Hi Michal,

Thanks for the patch! Some notes below...

On Sun, Jul 14, 2024 at 6:02 PM Michal Rostecki <vadorovsky@xxxxxxxxx> 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