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 <[email protected]> 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 > > [email protected] > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
signature.asc
Description: signature
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
