[Mesa-dev] [PATCH 5/8] i965: Add script to gen code for OA counter queries

[Dylan Baker]

Quoting Robert Bragg (2017-03-01 07:57:57)
[snip]
> >> +import xml.etree.ElementTree as ET
> >
> > use cElementTree, and please import as "et" instead of "ET", ET suggests to me
> > as a pythonista that this is a class not a module. (I know ElementTree is
> > capitalized, but that's a legacy thing, modern python style is modules are lower
> > case with _, classes are CamelCase)
> 
> Using cElementTree sounds good.
> 
> If I grep Mesa I see for 6 different uses of ElementTree with 5 out of
> 6 importing as ET. Really I just copied the style/name from other
> examples I found including the reference manual:
> https://docs.python.org/2/library/xml.etree.elementtree.html and ET
> does seem to be the common abbreviation used with ElementTree. I don't
> really have a strong oppinion here though and can just make the
> change.

It's not a big deal to switch from ET to et, python style has a changed a bit
over the years, before PEP8 was written python code was generally written with
Java style names, ElementTree is one of those older modules.

> 
> >
> >> +import argparse
> >> +import sys
> >
> > Also, please sort the imports
> 
> okey, can do; the python style guide says:
> 
> "Imports should be grouped in the following order:
> 
> 1. standard library imports
> 2. related third party imports
> 3. local application/library specific imports
> 
> You should put a blank line between each group of imports. "
> 
> I'm not actually sure if mesa's general style of alphanumerically
> sorting everything should override, but luckily my only third party
> import begins with x.

Yeah, generally you sort them by group

[snip]

> >> +def emit_fadd(tmp_id, args):
> >> +    c("double tmp" + str(tmp_id) +" = " + args[1] + " + " + args[0] + ";")
> >
> > "double tmp {tmp_id} = {args[1]} + {ags[0]};".format(tmp_id=tmp_id, args=args)
> >
> > This avoids the need to explicitly convert to str, and is much more readable.
> > I'd really recommend taking this approach for the rest of the file too, as it
> > will make things much more readable.
> 
> hmm, that seems like quite a bit more typing with some variables being
> written out three times this way :-/
> 
> maybe we could go with a variation like: "double tmp {0} = {1} +
> {2};".format(tmp_id, args[1], args[0])

Yup, that's fine too.

> 
> I've updated the emit_XYZ funcs to use .format() which seems more
> readable but I've still left quite a few uses of + based concatenation
> in places that I was less sure that it was more readable to use
> .format(). There was also at least once case that was made awkard
> since the string contains an open brace so I left it with + based
> concatenation.

Okay, you can use '{{' and '}}' to get a literal '{' and '}' in a formatter
string, but I understand why you might not want to. I appreciate the use of
str.format where it makes sense.

The other thing you can do, and it only makes sense in some places, is something
like this:
'MyCFunction_' + args[0] + '(' + args[1] + ')'
'MyCFunction_{0[0]}({0[1]})'.format(args)

again, use your best judgment on what it makes sense to use str.format

[snip]

> >> +/* Accumulation buffer offsets... */
> >> +.gpu_time_offset = 0,
> >> +.a_offset = 1,
> >> +.b_offset = 46,
> >> +.c_offset = 54,
> >> +""")
> >
> > textwrap.dedent is your friend for readability here.
> 
> yep, especially with this code further indented within a main() function
> 
> 
> Okey, I've got these changes made and working locally so I'll send out
> an update soon - thanks for the comments!
> 
> Br,
> - Robert

Thanks for making changes for me!

Dylan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170301/0128541f/attachment.sig>