[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 01/23] scripts: Add decodetree.py
From: |
Richard Henderson |
Subject: |
Re: [Qemu-devel] [PATCH 01/23] scripts: Add decodetree.py |
Date: |
Fri, 12 Jan 2018 06:54:35 -0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 |
On 01/12/2018 03:57 AM, Peter Maydell wrote:
> I asked on #qemu-devel for some review from people who are more
> familiar with Python than I am. One of the suggestions (from
> Marc-André Lureau) was to run pycodestyle on this and fix the
> (mostly coding style nits) reported by it. (pycodestyle may
> be called 'pep8' on older distros.)
Thanks, I'll have a look.
>> +# For unnamed_field, the first number is the least-significant bit position
>> of
>> +# the field and the second number is the length of the field. If the 's' is
>> +# present, the field is considered signed. If multiple unnamed_fields are
>> +# present, they are concatenated. In this way one can define disjoint
>> fields.
>
> This syntax lets you specify that fields other than the first one in
> a concatenated set are signed, like
> 10:5 | 3:s5
> That doesn't seem to me like it's meaningful. Shouldn't the signedness
> or otherwise be a property of the whole extracted field, rather than
> an individual component of it? (In practice creating a signed combined
> value is implemented by doing the most-significant component as sextract,
> obviously.)
You're right that it's not especially meaningful. But since I use deposit to
compose the pieces, any extraneous sign on a less significant component gets
smooshed. So nothing bad happens in the end. Which is why I decided not to
check.
> Do we syntax-check for accidentally specifying a field-def whose
> components overlap (eg "3:5 0:5")? I think we should, but I didn't
> see a check in a quick scan through the parsing code.
Probably not... something else for unit testing.
>> +# If any fixedbit_elt or field_elt appear then all 32 bits must be defined.
>> +# Padding with a fixedbit_elt of all '.' is an easy way to accomplish that.
>
> What is a format that doesn't specify the full 32 bits useful for?
> Do you have an example of one?
No. I'm not sure what I was thinking of there. I'm pretty sure the code
doesn't allow that.
> I notice in the generated code that all the trans_FOO functions
> are global, not file-local. That seems like it's going to lead
> to name clashes down the line, especially if/when we ever get
> to supporting multiple different target architectures in a single
> QEMU binary.
I was initially thinking that I'd have the translator functions in a different
file, and because of that they would of course have to be global. I had
thought far enough ahead to add command-line options to change the names and
prefixes.
But as it has turned out, putting the translator functions into the same file
has worked out well. I should probably rearrange this.
> Also from the generated code, "arg_incdec2_pred" &c don't follow
> our coding style preference for CamelCase for typedef names. On
> the other hand it's not immediately obvious how best to pick
> a camelcase approach for them...
Yeah, auto-generating names in different ways is tricky.
> A --help would be helpful (as would documenting the command
> line syntax in the comment at the top of the file).
Sure.
Thanks!
r~