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
