On 02/24/12 14:52, Danek Duvall wrote:
Shawn Walker wrote:
https://cr.opensolaris.org/action/browse/pkg/swalker/pkg-7145863/webrev/
...
_actions.c:
- line 496: don't you need a Py_DECREF(hash) here, too? And probably
before line 476, with the appropriate test for non-NULL and non-None?
Yes, thanks.
- In addition to Ed's comments on the decrefs in init_actions(), I'd add
that cache_class() might need to decref all of the previous action
class objects to be absolutely correct. Though I'll hazard that this
only happens when things are horribly wrong and we'll exit shortly
anyway.
Not if I understand you correctly. I'm borrowing the action class
references as GetItemString doesn't return a new reference. I'll add a
comment about that.
...
depend.py:
- line 42-50: sorted by rough order of occurrence, in order to make the
test on line 105 faster, I assume? Would a set be equally fast? If
not, a comment would be good.
Actually, as I mentioned to Ed, I didn't intend to change this. I've
reverted the ordering changes here. There was no real reason to change it.
...
file.py:
- line 592: is there no way to have "__init__ = types...." in the class
body?
Not as far as I can tell; the MethodType constructor needs a reference
to the class and the class isn't visible in the namespace until after
the full class definition has been parsed. I originally figured this
out based on information in the Cython FAQ. [1]
generic.py:
- line 276: maybe an assert() would be appropriate here?
No, I'm playing tricks here. Since the 'hash' attribute gets added to
'sattrs' at runtime, I take advantage of the fact that the only
attribute that's possibly in sattrs but not in self.attrs is the 'hash'
so I can just get that and move on.
Or are you suggesting I assert that k == 'hash' ?
I didn't think that was necessary as something must be horribly wrong
with Python itself if between line 255 and line 275 an attribute
suddenly goes missing from 'attrs'.
...
- line 1099 (removed): this allows us to have variant.arch=x and
variant.arch=y on the same action now?
We were already doing that during pkgdepend and pkglint stages; the only
reason it was working was because those tools used set() instead of
list() values for the attributes so it was fooling the check.
Rather than try to special-case those uses (and pay a significant
performance penalty) it seemed saner to simply change things to reflect
reality.
...
manifest.py:
- line 361: why all the changes from strip to rstrip? I would think it'd
be more critical to lstrip than to rstrip, and if the action parsing
code doesn't care about leading whitespace (it's not clear to me that
it doesn't), perhaps we can just get rid of the strips entirely?
The change was because these strips are all for things read from the
manifest where the left side should already be stripped, and only the
newline needs to be removed from the right side of the string.
The action parsing code does care about leading whitespace, but also
cares about trailing newlines.
Removing the strips causes manifest unit test failures. I also
experimented with having the action parsing code simply ignore leading
whitespace and trailing whitespace and it turned out to be slower (go
figure).
- line 609: why just for hardlinks now?
I've added a comment here already, but it's because all actions except
hardlinks have had the leading '/' stripped from paths for quite some
time now during action __init__. Why we never stripped the leading '/'
from paths for hardlinks is a historical mystery at this point. How
much it actually matters since we have stripped them from the manifests
that were written out for a very long time also remains uncertain.
pkgdep.py:
- line 134ff: while I understand that try/except is faster than
setdefault, the latter is a nicer idiom, and this is not a
speed-critical bit of code (as far as I can tell).
Per my comments to Brock, I was able to revert all changes to pkgdep.py,
so there are no changes here now.
-Shawn
[1]
http://wiki.cython.org/FAQ#HowdoIimplementasingleclassmethodinaCythonmodule.3F
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
