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

[Michal Rostecki <vadorovsky@gmail.com>]

On 16.07.24 09:44, Miguel Ojeda wrote:
> On Mon, Jul 15, 2024 at 6:12 PM Michal Rostecki <vadorovsky@gmail.com> 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.
> 

Alright, I will go with Trevor's suggestion and provide a `display()` 
method via an extension trait.

>> 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.
> 

Fair enough. I will provide `from_utf8_unchecked_mut()` as a part of 
`CStrExt` in the next version.

>> 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 :)
> 

`stringify!` is not the only case where I ended up using `c_str!`. After 
addressing Björn's suggestion about taking Rust strings as arguments in 
`new_mutex!`, `new_condvar!` etc., `optional_name!` is also using 
`c_str!` in the following way:

   macro_rules! optional_name {
       () => {
           $crate::c_str!(::core::concat!(::core::file!(), ":", 
::core::line!()))
       };
       ($name:literal) => {
           $crate::c_str!($name)
       };
   }


So I think that leaving `c_str!` still makes sense, unless you have 
other suggestions, which are still easily applicable there. :)

> Cheers,
> Miguel

Cheers,
Michal