Messages in this thread | | | Date | Sun, 26 Feb 2023 14:13:10 -0800 | From | Boqun Feng <> | Subject | Re: [PATCH 5/5] rust: error: Add from_kernel_result!() macro |
| |
On Sun, Feb 26, 2023 at 09:59:25PM +0100, Miguel Ojeda wrote: > On Sun, Feb 26, 2023 at 7:17 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > My preference to function instead of macro here is because I want to > > avoid the extra level of abstraction and make things explict, so that > > users and reviewers can understand the API behavior solely based on > > Rust's types, functions and closures: they are simpler than macros, at > > least to me ;-) > > There is one extra problem with the macro: `rustfmt` does not format > the contents if called with braces (as we currently do).
Interesting, sounds like a missing feature in `rustfmt` or maybe we don't use the correct config ;-)
> > So when I was cleaning some things up for v8, one of the things I did > was run manually `rustfmt` on the blocks by removing the macro > invocation, in commit 77a1a8c952e1 ("rust: kernel: apply `rustfmt` to > `from_kernel_result!` blocks"). > > Having said that, it does format it when called with parenthesis > wrapping the block, so we could do that if we end up with the macro. > > > First, I think the macro version here is just a poor-man's try block, in > > other words, I'd expect explicit use of try blocks intead of > > `from_kernel_result` when the feature is ready. If that's the case, we > > need to change the use sites anyway. > > Yeah, if we eventually get a better language feature that fits well, > then we should use it. > > > Do both implementation share the same behavior? > > Yeah, a `return` will return to the outer caller in the case of a > `try` block, while it returns to the closure (macro) in the other > case. Or do you mean something else? >
"Yeah" means they have different behaviors, right? ;-)
Thanks for confirming and I think you get it, but just in case for others reading this: if we use the macro way to implement `from_kernel_result` as in this patch:
macro_rules! from_kernel_result { ($($tt:tt)*) => {{ $crate::error::from_kernel_result_helper((|| { $($tt)* })()) }}; }
and later we re-implement with try blocks:
macro_rules! from_kernel_result { ($($tt:tt)*) => {{ $crate::error::from_kernel_result_helper(try { $($tt)* }) }}; }
the `from_kernel_result` semantics will get changed on the `return` statement inside the macro blocks.
And this is another reason why we want to avoid use macros here. Code example as below:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=58ea8b95cdfd6b053561052853b0ac00
`foo_v1` and `foo_v3` has the exact same function body, but behave differently.
> In that case, I think one could use use a labeled block to `break` > out, not sure if `try` blocks will allow an easier way. > > We have a case of such a `return` within the closure at `rust/rust` in > `file.rs`:
Thanks for finding an example! Means we did use return.
For this particular API, I'd say function right now, `try` blocks if avaiable.
Regards, Boqun
> > from_kernel_result! { > let off = match whence as u32 { > bindings::SEEK_SET => SeekFrom::Start(offset.try_into()?), > bindings::SEEK_CUR => SeekFrom::Current(offset), > bindings::SEEK_END => SeekFrom::End(offset), > _ => return Err(EINVAL), > }; > ... > Ok(off as bindings::loff_t) > } > > Cheers, > Miguel
| |