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~
