On 27.8.2015 12:17, Petr Viktorin wrote:
On 08/27/2015 09:07 AM, Jan Cholasta wrote:
On 26.8.2015 12:15, Petr Viktorin wrote:
On 08/26/2015 09:47 AM, Jan Cholasta wrote:
On 25.8.2015 15:05, Petr Viktorin wrote:
On 08/25/2015 12:39 PM, Christian Heimes wrote:
On 2015-08-24 17:31, Petr Viktorin wrote:
0701.2-Use-Python3-compatible-dict-method-names
NACK
Why are you replacing iteritems() with items() instead of using
six.iteritems()?
It looks cleaner, and it will be easier to clean up after six is
dropped.
Also, the performance difference is negligible if the whole thing is
iterated over. (On small dicts, which are the majority of what
iteritems
was used on, items() is actually a bit faster on my machine.)
Right, for small dicts the speed difference is negligible and
favors the
items() over iteritems(). For medium sized and large dicts the
iterators
are faster and consume less memory.
I'm preferring iterator methods everywhere because I don't have to
worry
about dict sizes.
0710.2-Modernize-use-of-range
NACK
Please use six.moves.range. It defaults to xrange() in Python 2.
Why? What is the benefit of xrange in these situations?
Like with iteritems in 0701, when iterating over the whole thing, the
performance difference is negligible. I don't think a few
microseconds
outside of tight loops are worth the verbosity.
It's the same reasoning as in 0701. As long as you have a small range,
it doesn't make a difference. For large ranges the additional memory
usage can be problematic.
In all above cases the iterator methods and generator functions are a
safer choice. A malicious user can abuse the non-iterative methods for
DoS attacks. As long as the input can't be controlled by a user and
the
range/dict/set/list is small, the non-iterative methods are fine. You
have to verify every location, though.
Keep in mind that for dicts, all the data is in memory already (in the
dict). Are you worried about DoS from an extra list of references to
existing objects?
I'm usually too busy with other stuff (aka lazy) to verify these
preconditions...
I changed one questionable use of range. The other ones are:
ipalib/plugins/dns.py: for i in range(len(relative_record_name)
(max. ~255, verified by DNSName)
ipaserver/install/dnskeysyncinstance.py: for _ in range(0, 16))
(16)
ipaserver/install/ipa_otptoken_import.py: for i in range(1, blocks +
1):
(about 7)
ipaserver/install/ipa_otptoken_import.py: for k in
range(mac.digest_size):
(16)
ipatests/data.py: for d in range(256))
(256)
Plus tests, where it's rarely above 10.
I have just pushed Michael's python-krbV removal patch, which conflicts
with your patches. Could you please rebase them on top of current
master?
Sure, here they are.
Patches 696-699: ACK
Patch 700: There are some error messages which contain basestring. Do we
want to fix these as well?
No, I think it's okay as a term. When Python 2 is dropped it can be
shortened to str.
Patch 701: Since we are using python-six, shouldn't "six.PY2" be used
instead of "sys.version_info < (3, 0)"?
Right, it looks a bit better.
Patches 702-709: ACK
Patch 710: There are some "xrange"s in ipapython/p11helper.py and
ipatests/test_xmlrpc/test_add_remove_cert_cmd.py.
Thanks, fixed.
Patch 711: ACK
Thanks, ACK.
Christian, if you don't have any objections, I will push the patches.
--
Jan Cholasta
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
