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

[Ilia Mirkin]

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.

>
>
> 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).

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