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

[Ian Romanick]

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

> 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