On Thu, Apr 10, 2008 at 03:00:06PM -0700, Dan Price wrote: > On Thu 10 Apr 2008 at 10:54AM, Danek Duvall wrote: > > On Thu, Apr 10, 2008 at 01:25:47AM -0700, Dan Price wrote: > > > > > http://cr.opensolaris.org/~dp/ips-perf > > > generic.py > > > > - line 177: is there anything about orderdict that's instance-specific? > > Couldn't we save a teeny bit of time and space by making it specific > > either to the Action class or even the pkg.actions module? > > I thought that it was essentially a static element of the class as > declared?
I'm a bit fuzzy on that. Leave it, we'll look at it later. > > - line 233: inconsistent use of single-quotes vs backslash-escaping. > > Would it be any slower to use %-expandos -- it'd likely be easier to > > read. > > When I was bit twiddling, I found % expandos to be enough slower than > string concatenation to make a difference in hot paths like this one, > although my testing is admittedly limited. That's fine; I assumed that was the case. > > image.py > > > > - line 896, 902: I'd probably rename gen_installed_pkgs() to > > installed_pkgs(), and gen_installed_pkgs_forreal() to > > gen_installed_pkgs(). That means renaming all the clients, but I think > > it's a better naming scheme. We can postpone that if you like. > > Yes, let's please defer that. I thought the convention was that > things which can be used as generators had gen_ in them? I'm waffling. > > manifest.py > > > > - line 257: I guess I was thinking that the actions should be agnostic in > > regards to the information they carried, while the manifest was more > > part of the system, where the idea that paths shouldn't start with the > > root filesystem was more relevant, and that the "path" attribute was > > universal across actions that represented filesystem objects. > > Ok-- so, should I push this out into the File, Dir, etc. actions? Since > path is commonly specified, this isn't a performance concern, more just > a question I had when I found this code. I'll remove the comment. Yeah, leave the code as-is. We can revisit later if it remains confusing. > > - line 267: won't this have problems if self.size isn't set first? > > I may be confused-- but self.size is set to 0 in the constructor. It is. Though if you call set_content() more than once ... > > - line 315: what if this fails? > > Well, we'll throw. Pretty sure this is the same as the previous > behavior from before. Okay. When I saw you were touching that code, I was hoping you were fixing that. :) Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
