[Mesa-dev] [PATCH 22/41] glapi: gl_XML.py: convert gl_api.functionIterateByOffset to a generator

[Dylan Baker]

Quoting Ian Romanick (2016-04-19 16:13:51)
> On 04/01/2016 11:47 AM, Ilia Mirkin wrote:
> > On Fri, Apr 1, 2016 at 2:41 PM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> >> Quoting Ilia Mirkin (2016-04-01 08:46:19)
> >>> IMHO this is still quite fancy and unnecessarily complicated.
> >>>
> >>> How about
> >>>
> >>>   temp = dict((f.offset, f) for f in self.functions_by_name.itervalues())
> >>>   return (temp[offset] for offset in sorted(temp))
> >>>
> >>> That's all that's happening there, right? This lets you not use yield
> >>> (which is confusing to most people, as Brian points out), but still
> >>> uses a generator to avoid creating that second list, behind the
> >>> scenes, while staying understandable to people who understand list
> >>> comprehensions (which, hopefully, anyone using python by now does...
> >>> they've been around since at least Python 2.2)
> >>>
> >>> [BTW, this is not a complete review of the series... just looking at
> >>> this patch because Brian happened to reply to it.]
> >>
> >> I knew there must be an easier way to do this, I had a different
> >> implementation that was simpler, but didn't sort correctly, before this
> >> one, and I wasn't really happy with this.
> >>
> >> I don't think we actually need to care about max_offset do we? Since
> >> that was more an implementation detail of the original function, I'll
> >> rebase out the previous patch too.
> > 
> > Nope, it's just there to size the array. [From what I can tell.]
> > 
> >> I don't think yours is exactly right though, since it doesn't handle the
> >> offset == -1, but that's trivial.
> > 
> > Er, right, of course. Forgot :) And it's a pretty crucial detail.
> > 
> >> Something like this(?):
> >>
> >> temp = [(f.offset, f) for f in self.functions_by_name.itervalues()
> >>         if f.offset != -1]
> >> return (func for _, func in sorted(temp))
> > 
> > And this has the nice advantage of not creating a dict for no reason.
> > There's the hypothetical issue of 2 functions having the same offset,
> > but... I don't think that's possible in practice.
> 
> Nope.  Offsets are either uniquely, statically assigned in the XML, or
> they're assigned dynamically by the scripts.  If two functions are
> supposed to have the same location, they must be marked as aliases in
> the XML.
> 
> >> I guess we could make one generator comprehension out of it, but
> >> performance isn't that critical (though this does seem to be slightly
> >> faster).
> >>
> >> return (f for f in sorted(self.functions_by_name.itervalues(),
> >>                           key=lambda f: f.offset)
> >>         if f.offset != -1)
> >>
> >>
> >> Which do you think is more readable?
> > 
> > I like the first one, esp since the list it builds for sorting is
> > going to be *much* smaller (I could be wrong, but I think the majority
> > of functions don't have an explicit offset, i.e. end up with -1).
> 
> I like the first one too.  I was pretty sure there was a way to do it
> like that... but my Python skills aren't quite good enough.  I'm glad I
> decided to read the rest of the thread before searching Python websites. :)

Cool, I'll add a comment here that the data guarantees that the offsets
shouldn't collide, and use the first one when I spin the v2.

> 
> > Perhaps Brian should make the call on understandability though, as
> > someone not familiar with the ins and outs of Python? [I,
> > unfortunately(?), crossed that line quite some years back --
> > generators in Py2.4 were the cool new thing.]
> > 
> > Thanks,
> > 
> >   -ilia
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160419/e242b265/attachment.sig>