Re: [PATCH 15/23] rust: introduce alternative implementation of offset_of!

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/3/24 10:54, Junjie Mao wrote:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> From: Junjie Mao <junjie.mao@hotmail.com>
>>
>> offset_of! was stabilized in Rust 1.77.0.  Use an alternative implemenation
>> that was found on the Rust forums, and whose author agreed to license as
>> MIT for use in QEMU.
>>
>> The alternative allows only one level of field access, but apart
>> from this can be used just by replacing core::mem::offset_of! with
>> qemu_api::offset_of!.
>>
>> The actual implementation of offset_of! is done in a declarative macro,
>> but for simplicity and to avoid introducing an extra level of indentation,
>> the trigger is a procedural macro #[derive(offsets)].
>>
>> The procedural macro is perhaps a bit overengineered, but it helps
>> introducing some idioms that will be useful in the future as well.
>>
>> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
>> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Generally looks good to me. Thanks for integrating this!
> 
> It seems Rust does not have builtin support for unit tests expecting
> compilation failures. There is a crate (named trybuild [1]) for that
> purpose but it requires introducing a dozen of new dependencies (see
> below). Do you think it worth the effort? If so, I can take a closer
> look and cook something for initial review (probably post 9.2).
> 
>      trybuild v1.0.101
>      ├── glob v0.3.1
>      ├── serde v1.0.210
>      ├── serde_derive v1.0.210 (proc-macro)
>      │   ├── proc-macro2 v1.0.84 (*)
>      │   ├── quote v1.0.36 (*)
>      │   └── syn v2.0.66 (*)
>      ├── serde_json v1.0.132
>      │   ├── itoa v1.0.11
>      │   ├── memchr v2.7.4
>      │   ├── ryu v1.0.18
>      │   └── serde v1.0.210
>      ├── target-triple v0.1.3
>      ├── termcolor v1.4.1
>      └── toml v0.8.19
>          ├── serde v1.0.210
>          ├── serde_spanned v0.6.8
>          │   └── serde v1.0.210
>          ├── toml_datetime v0.6.8
>          │   └── serde v1.0.210
>          └── toml_edit v0.22.22
>              ├── indexmap v2.6.0
>              │   ├── equivalent v1.0.1
>              │   └── hashbrown v0.15.0
>              ├── serde v1.0.210
>              ├── serde_spanned v0.6.8 (*)
>              ├── toml_datetime v0.6.8 (*)
>              └── winnow v0.6.20
> 
> [1] https://docs.rs/trybuild/latest/trybuild/
> 
> [snip]
>> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
>> index a4bc5d01ee8..c2ea22101e4 100644
>> --- a/rust/qemu-api-macros/src/lib.rs
>> +++ b/rust/qemu-api-macros/src/lib.rs
>> @@ -3,8 +3,34 @@
>>   // SPDX-License-Identifier: GPL-2.0-or-later
>>
>>   use proc_macro::TokenStream;
>> -use quote::quote;
>> -use syn::{parse_macro_input, DeriveInput};
>> +use proc_macro2::Span;
>> +use quote::{quote, quote_spanned};
>> +use syn::{
>> +    parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
>> +    Fields, Ident, Type, Visibility,
>> +};
>> +
>> +struct CompileError(String, Span);
>> +
>> +impl From<CompileError> for proc_macro2::TokenStream {
>> +    fn from(err: CompileError) -> Self {
>> +        let CompileError(msg, span) = err;
>> +        quote_spanned! { span => compile_error!(#msg); }
> 
> The documentation [2] says "there should be no space before the =>
> token" and that is by intention to tell that `span` is "evaluated in the
> context of proc macro" while those after the arm "in the generated
> code". Should we follow that convention (even though the extra white
> space does not impact building)?

Ah, forgot to reply about this.  Personally I think it's clear enough 
with the space around both sides of "=>", but if there's agreement on 
removing the space I don't oppose it.

Paolo