On Fri, Apr 1, 2016 at 2:41 PM, Dylan Baker <baker.dyla...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
