On 07/ 8/10 10:30 AM, [email protected] wrote:
Hi Shawn,
On Wed, Jul 07, 2010 at 06:58:05PM -0700, Shawn Walker wrote:
Greetings,
The following webrev contains changes for the following issues:
16238 client api should ignore invalid and not yet supported packages
1142 match processing code replicated in various places
webrev:
http://cr.opensolaris.org/~swalker/pkg-16328/
This looks good. I just have a few nits:
api.py:
- line 1032 (and others): I've seen a bunch of instances of this:
while tgt is not None:
can this be re-written as?
while tgt:
Is it ever possible for this code 'ren_stems.get(stem, None)' to
return a stem that's valid but evaluates to False? If not, it's
simpler to omit the 'is' part of the check.
In Python, testing for identity is much faster than testing for
"truthness" or "equality" because it simply compares memory addresses.
Since this code is a hot path, I've opted to take the Python developer's
recommendations to test for identity.
...
- line 1530: Should this be, "if omit_package is True" instead?
Yes, I missed this one.
- lines 1590 - 1769: Similar question again, there seem to be
gratuitious use of the 'is' comparison in this function too. Can we
let Python check that local and unsupported evaluate to False
See above.
without using is? Do dependencies and mfst have valid values that
evaluate to False but that aren't None?
No, dependencies and mfst will never have a valid value that isn't None.
transport.py:
- lines 921 and 1119: This can be simplified to 'if verified' instead.
Since this isn't a hot path, that seems fine.
manifest.py:
- line 296: It would simplify this code if you initialized this
to an empty list instead. That would mean that you could remove
lines 316-319. (How much overhead is there for allocating an empty
list? I would expect the yielding of results to dominate the time
spent in this function). This also means that you could change line
324 to 'if errors' since it will only return True if there are items
in the list.
Since the error case is not common, using errors = None avoids a
significant number of pointless memory allocations for a list that will
never be populated.
Cheers,
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
