On 14.07.24 19:30, Miguel Ojeda wrote:
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?
Alright, I will remove the second part of the sentence.
- It does not implement `Display` (but implements `Debug`).
One question that comes up when reading this is: are we losing
`Display`'s output form?
Yes, we are losing the `Display` trait implementation by switching to
`core::ffi::CStr`.
I was thinking whether I should keep `kernel::str::CStr` as a wrapper,
just to keep the `Display` implementation. I could still do that if you
want. I'm also open for other solutions.
The reason why I decided to not do that and go ahead without `Display`
is that it was used only in rust/kernel/kunit.rs inside `kunit_assert`,
for formatting the file and path the error message. This diff:
@@ -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.
Also, for clarity, please mention if there is a difference in the
output of the `Debug` ones.
There isn't any difference, I will mention that.
- 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.
Regarding `DerefMut` implementation for `CString` - we don't need it. Or
at least - removing it (after my CStr patch), does not break anything.
If that's fine for you, I'm going to remove it in v2 all together.
About why having `&mut CStr` is undesirable - I will try to find better
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.
- 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?
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?
[0] https://github.com/Rust-for-Linux/linux/issues/1075
+ // 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.
Good point, I will fix the comment.
-//! 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.
I already replied to Björn - names passed to `new_work!`, `new_mutex!`
are immediatelly passed to FFI calls and are not used in the Rust code
internally, so I prefer to keep them as C strings rather than Rust strings.
Cheers,
Miguel
Cheers,
Michal
