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