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@gmail.com 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