Re: [Qemu-devel] [PATCH 01/23] scripts: Add decodetree.py

[Richard Henderson <richard.henderson@linaro.org>]

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~