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:
Why are you replacing iteritems() with items() instead of using

It looks cleaner, and it will be easier to clean up after six is
Also, the performance difference is negligible if the whole thing is
iterated over. (On small dicts, which are the majority of what
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
are faster and consume less memory.

I'm preferring iterator methods everywhere because I don't have to
about dict sizes.

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
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
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

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))

ipaserver/install/ipa_otptoken_import.py: for i in range(1, blocks +
(about 7)

ipaserver/install/ipa_otptoken_import.py: for k in

ipatests/data.py: for d in range(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

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

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:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to