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

[Miguel Ojeda]

On Mon, Jul 15, 2024 at 6:12 PM Michal Rostecki <vadorovsky@xxxxxxxxx> wrote:
>
> @@ -71,11 +75,11 @@ macro_rules! kunit_assert {
>                   //
>                   // This mimics KUnit's failed assertion format.
>                   $crate::kunit::err(format_args!(
> -                    "    # {}: ASSERTION FAILED at {FILE}:{LINE}\n",
> +                    "    # {:?}: ASSERTION FAILED at {FILE:?}:{LINE:?}\n",
>                       $name
>                   ));
>                   $crate::kunit::err(format_args!(
> -                    "    Expected {CONDITION} to be true, but is false\n"
> +                    "    Expected {CONDITION:?} to be true, but is false\n"
>                   ));
>
> The only practical difference in switching from `Display` to `Debug`
> here is that the fallback kunit error messages are going to include
> quotation marks around conditions, files and lines.

That is a fairly important difference -- the messages are intended to
match the C KUnit ones.

Especially the file:line notation -- I don't think a user would expect
to have quotes there (regardless of KUnit).

In general, even if we didn't need it right now, I think it is
something we will need sooner or later.

> wording. My general point is that I've never seen `&mut str` being
> exposed in any core/std API to the external user, mutation usually
> implies usage of an owned String.

Not sure what you mean by exposed, but even if `&mut str`'s methods do
not count (used via `String`), there is also
`from_utf8_unchecked_mut()` that returns one, which is essentially the
same idea as what we had here.

So I am not sure about the "The rule of Rust std" part in the new
commit messages. And, to be clear, while the Rust standard library is
a good reference to look into, sometimes we want/need to do things
differently anyway (which is not really the case here given
`from_utf8_unchecked_mut()`, I would say).

In this case, regardless of the standard library, personally I would
have preferred to have a non-public function, but still have it (and
properly documented), rather than open code the `unsafe` block with
the casts.

> I think the best solution would be leaving `c_str` macro for that. The
> reason why I removed it is that the GitHub issue[0] mentions its
> removal. But for that case, I think it makes sense to leave it. What do
> you think?

Perhaps the issue was only taking into account the C string literal
case? Benno may know more.

Generally speaking, replacing a clean line with a bigger `unsafe`
block is something to be avoided.

Maybe a `c_stringify!` is what we need :)

Cheers,
Miguel