[issue10303] small inconsistency in tutorial
New submission from Malte Helmert : Section "3.1.2. Strings" says "*Once again*, the print() function produces the more readable output.", but as far as I can see (or grep), this is the first time that this aspect of print() is mentioned. -- assignee: d...@python components: Documentation messages: 120362 nosy: d...@python, maltehelmert priority: normal severity: normal status: open title: small inconsistency in tutorial versions: Python 3.2 ___ Python tracker <http://bugs.python.org/issue10303> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue10304] error in tutorial triple-string example
New submission from Malte Helmert :
>From Section 3.1.2 of the tutorial:
print("""
Usage: thingy [OPTIONS]
-hDisplay this usage message
-H hostname Hostname to connect to
""")
produces the following output:
_
Usage: thingy [OPTIONS]
-hDisplay this usage message
-H hostname Hostname to connect to
_
That doesn't quite match the behaviour: there should be an extra blank line
prepended to the output.
--
assignee: d...@python
components: Documentation
messages: 120364
nosy: d...@python, maltehelmert
priority: normal
severity: normal
status: open
title: error in tutorial triple-string example
versions: Python 3.2
___
Python tracker
<http://bugs.python.org/issue10304>
___
___
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue10304] error in tutorial triple-string example
Malte Helmert added the comment:
Here's a related one if you want to get the sample output really 100% correct.
In the last example of Section 3, the output in
{>>> a, b = 0, 1
>>> while b < 1000:
... print(b, end=' ')
... a, b = b, a+b
...
1 1 2 3 5 8 13 21 34 55 89 144 233 377 610 987}
should have an extra space at the end. (Granted, this is very very minor, but
this may make a Python 2.x oldtimer like me wonder if print's end=" " does the
same as old Python 2.x's "print b," including suppression of the "softspace".)
--
___
Python tracker
<http://bugs.python.org/issue10304>
___
___
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue10327] Abnormal SSL timeouts when using socket timeouts - once again
Malte Helmert added the comment: I checked if issue1153016 has reappeared for me (Ubuntu, Python 2.6.6), but it hasn't. Both the urllib and the imaplib examples given there work fine for me. Or at least opening the connections works fine for me, which it didn't at the time of issue1153016. But you say you have random errors, so maybe I need to exercise this more heavily. Can you attach a test script that can be used to make the bug appear? -- ___ Python tracker <http://bugs.python.org/issue10327> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue10304] error in tutorial triple-string example
Malte Helmert added the comment:
> 1. Rather than add a blank line to the output, the input should have
> the newline suppressed with \ (which has been done in previous
> examples).
> print("""\
I think that would be didactically bad after just mentioning that newlines in
triple-quoted strings don't have to be escaped etc.
I think this would confuse the reader.
Better use print("""Usage...
> In interactive use, the interpreter will go to a new line anyway
> before printing the >>> prompt.
It won't (at least in Python 3.1). I just tried it.
> The intent and effect of end=' ' is that the outputs are all on one
> line (as with 2.x print) instead of each on a separate line
Sure. But I think that in an example that is essentially about whitespace
produced by print, the actual whitespace in the output should match the actual
behaviour of print. One why to get rid of this problem altogether is to add a
'print "done!"' or whatever at the end, or use a different separator from " ".
--
___
Python tracker
<http://bugs.python.org/issue10304>
___
___
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue10304] error in tutorial triple-string example
Malte Helmert added the comment: I see. (The tutorial really talks about the interactive interpreter though -- I don't think IDLE has been mentioned yet.) -- ___ Python tracker <http://bugs.python.org/issue10304> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert <[EMAIL PROTECTED]> added the comment: David, not sure what you are commenting on. Are you commenting on one of the patches? The patches do contain those divisions, of course; you can also run the attached unit test to verify that the patches work for you. ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Changes by Malte Helmert <[EMAIL PROTECTED]>: Removed file: http://bugs.python.org/file9497/test_times.py ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Changes by Malte Helmert <[EMAIL PROTECTED]>: Removed file: http://bugs.python.org/file9501/os_times.PATCH ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Changes by Malte Helmert <[EMAIL PROTECTED]>: Removed file: http://bugs.python.org/file9506/test_posix.PATCH ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Changes by Malte Helmert <[EMAIL PROTECTED]>: Removed file: http://bugs.python.org/file9515/test_posix2.PATCH ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Changes by Malte Helmert <[EMAIL PROTECTED]>: Removed file: http://bugs.python.org/file9516/test_posix3.PATCH ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Changes by Malte Helmert <[EMAIL PROTECTED]>: Removed file: http://bugs.python.org/file9517/test_posix4.PATCH ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1679] tokenizer permits invalid hex integer
Malte Helmert added the comment:
I can find three places where "0x" is accepted, but probably shouldn't:
1. Python's tokenizer:
>>> 0x
0
>>> 0xL
ValueError: invalid literal for long() with base 16: '0xL'
=> I think these should both be syntax errors.
2. int builtin:
>>> int("0x", 0) == int("0x", 16) == 0
True
>>> long("0x", 0)
Traceback (most recent call last):
File "", line 1, in
ValueError: invalid literal for long() with base 16: '0x'
>>> long("0x", 16)
Traceback (most recent call last):
File "", line 1, in
ValueError: invalid literal for long()
=> The long behaviour looks right to me, and I think the int behaviour
should match it.
3. tokenize module:
This currently accepts "0x" and "0xL" as single tokens. The obvious fix
would lead to these two being reported as two separate tokens ("0":
NUMBER, "x": NAME; "0": NUMBER, "xL": NAME), as it currently does for
other cases where a name follows a number like "23cats". However, this
is not quite what Python's parser does, which returns an error token
instead. (Fortunately, name after number appears to be a syntax error
everywhere, so it doesn't really affect the behaviour; a syntax error
occurs either way.)
--
nosy: +maltehelmert
__
Tracker <[EMAIL PROTECTED]>
<http://bugs.python.org/issue1679>
__
___
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1679] tokenizer permits invalid hex integer
Malte Helmert added the comment: Here's a patch that fixes case 1: >>> 0x File "", line 1 0x ^ SyntaxError: invalid token >>> 0xL File "", line 1 0xL ^ SyntaxError: invalid token Added file: http://bugs.python.org/file9221/PATCH-1.diff __ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1679> __ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1679] tokenizer permits invalid hex integer
Malte Helmert added the comment: And here's a patch that fixes case 3. Added file: http://bugs.python.org/file9222/PATCH-3.diff __ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1679> __ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1679] tokenizer permits invalid hex integer
Malte Helmert added the comment:
And here's a patch for case 2 (int) conversion. There is still a slight
inconsistency in error reporting (base 0 vs. base 16) between int and
long, but I'd see this as long's fault:
>>> int("0x", 0)
Traceback (most recent call last):
File "", line 1, in
ValueError: invalid literal for int() with base 0: '0x'
>>> int("0x", 16)
Traceback (most recent call last):
File "", line 1, in
ValueError: invalid literal for int() with base 16: '0x'
>>> long("0x", 0)
Traceback (most recent call last):
File "", line 1, in
ValueError: invalid literal for long() with base 16: '0x'
>>> long("0x", 16)
Traceback (most recent call last):
File "", line 1, in
ValueError: invalid literal for long() with base 16: '0x'
The patch is not pretty because it duplicates a lot of code, but it's
probably easier to see what was changed that way. I'll add a prettier
patch soon.
Added file: http://bugs.python.org/file9224/PATCH-2a.diff
__
Tracker <[EMAIL PROTECTED]>
<http://bugs.python.org/issue1679>
__
___
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1679] tokenizer permits invalid hex integer
Malte Helmert added the comment: This is a cleaner version of PATCH-2a.diff in the sense that the resulting code contains less duplication. The disadvantage is that it applies more structural changes to PyOS_strtoul, so may be harder to merge with other changes. Added file: http://bugs.python.org/file9226/PATCH-2b.diff __ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1679> __ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1679] tokenizer permits invalid hex integer
Malte Helmert added the comment: Added tests to test_grammar, test_builtin and test_tokenize. Added file: http://bugs.python.org/file9229/PATCH-TESTS.diff __ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1679> __ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1433694] normalize function in minidom unlinks empty child nodes
Malte Helmert added the comment:
I can reproduce the bug on trunk (r60511). At first I thought the
behaviour might be caused by the testcase removing items from the
children list while iterating over it, but this is not the case; the
exception is raised upon the first removal already.
Here is a shorter testcase:
from xml.dom import minidom
def testme(xmltext):
node = minidom.parseString(xmltext).documentElement
for child in node.childNodes:
if child.nodeValue == "t":
child.nodeValue = ""
node.normalize()
child = node.firstChild
while child:
next = child.nextSibling
if child.nodeType == child.TEXT_NODE and child.nodeValue == "":
node.removeChild(child)
child = next
return node
print testme("tt").toxml()
print testme("t").toxml()
The second call to testme fails with an xml.dom.NotFoundErr, but it
should succeed and print "".
While this appears to be a genuine bug, I don't agree with the proposed
fix. I'm not sure if calling removeChild (which mutates self.childNodes)
from within normalize (which iterates over self.childNodes at that time)
is safe, and even if it is, it would make normalize O(N^2) (not taking
into account recursion) where it should be O(N).
--
nosy: +maltehelmert
_
Tracker <[EMAIL PROTECTED]>
<http://bugs.python.org/issue1433694>
_
___
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1433694] normalize function in minidom unlinks empty child nodes
Malte Helmert added the comment: OK, I think I found the root cause. Node.normalize regenerates the list of children L with their previousSibling/nextSibling references from scratch; however, it fails to set the nextSibling reference for the very last element of L to None at the end. This is necessary iff the last node in the original child list is removed. The attached patch should solve the problem. In cases like this, should I also add a regression test? Added file: http://bugs.python.org/file9345/MINIDOM-PATCH _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1433694> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1433694] normalize function in minidom unlinks empty child nodes
Malte Helmert added the comment:
Here is a minimal testcase to more clearly expose the root of the
problem, in case a regression test is needed. Without the patch, the
assertion fails.
==
from xml.dom import minidom
node = minidom.parseString("t").documentElement
node.childNodes[1].nodeValue = ""
node.normalize()
assert node.childNodes[-1].nextSibling == None
==
_
Tracker <[EMAIL PROTECTED]>
<http://bugs.python.org/issue1433694>
_
___
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1433694] normalize function in minidom unlinks empty child nodes
Changes by Malte Helmert: -- versions: +Python 2.6 -Python 2.4 _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1433694> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: I'm attaching a test script (test_times.py) that forks a child which runs for 5 seconds, waits for the child, then prints the time taken by the child according to os.times(). I have a machine where os.times() reproducably reports that 8.33 seconds have been spent, although indeed only 5 seconds pass: == # time python test_times.py 8.333 real0m5.018s user0m5.008s sys 0m0.008s == I don't know which characteristics of the machine are causing this. -- nosy: +maltehelmert Added file: http://bugs.python.org/file9497/test_times.py _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Here's three tests with different pythons on the same machine: # ./python ../test_times.py 8.333 # python ../test_times.py 8.333 # python2.5 ../test_times.py 5.0 The first Python is current trunk, built just now. The second Python is the vendor-installed (SuSE) Python 2.5. The third Python is a Python 2.5 I installed manually from source some time ago. Strange that it would differ from the second; it appears to be the same revision as the second from the greeting message. Anyway, I can reproduce the error in the trunk, which is good. This is a 64-bit SuSE Linux machine (Xeon X5355 @ 2.66GHz). _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: I'm attaching a patch against trunk that fixes the problem for me (os_times.PATCH). This uses the sysconf values when HAVE_SYSCONF is defined, and otherwise falls back on the old behaviour (use HZ if that is defined, 60 otherwise). I'm not sure if this is stylistically ok (#ifdefs inside a function, etc.), so I'd appreciate comments. Should I add a test case for the test suite? Added file: http://bugs.python.org/file9501/os_times.PATCH _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Another comment: Since the fallback value of 60 was wrong in the past, it may likely be wrong in the future. Should that fallback be removed and replaced by a compile-time error? And is the "HZ" fallback necessary at all? I don't know enough about Posix to know whether or not HAVE_SYSCONF should be implied by HAVE_TIMES. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Here is a patch that adds a test case to test_posix.py. This is ignored on Windows, as it requires fork. There is a trade-off: If WAIT_TIME isn't large enough, small irregularities in the process scheduler might cause this to fail. If it is too large, the unit tests will take long for everyone. Currently, WAIT_TIME is 0.1 seconds and the tolerance in the assertion is about 5%, which works well for me. Not sure how this behaves on very busy machines. Added file: http://bugs.python.org/file9506/test_posix.PATCH _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Using 1.0 would certainly be more robust. I wasn't sure if a slow-down of "make test" by 1 second just for this one bug would be acceptable. Regarding the fork, when I first encountered this bug, it was in the context of measuring the runtime of child processes, so that's what I tried to reproduce. But looking at the code, the bug should occur just as well with the running process itself. So you're right; one could just busy-wait for a second and then look at times()[0] and times()[1]. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Note: My original unit test fails to take account that previous unit tests may have spawned child processes. The correct behaviour is of course to call os.times() before and after the fork and compute the difference. I'm not uploading a modified patch because from previous comments it looks like a different test will be used anyway. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue2170] rewrite of minidom.Node.normalize
New submission from Malte Helmert: In the discussion of #1433694 on the #python-dev channel, it was observed that the normalize method of minidom.Node could take some refactoring. A patch is attached. -- components: XML files: minidom.diff messages: 62794 nosy: maltehelmert severity: normal status: open title: rewrite of minidom.Node.normalize type: feature request versions: Python 2.6 Added file: http://bugs.python.org/file9513/minidom.diff __ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue2170> __ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Attaching a new test (test_posix2.PATCH) that doesn't fork and fixes the problem with the previous test not taking previously elapsed time into account. This supersedes test_posix.PATCH. I left the wait time at 0.1; if we stay within the same process, this should be large enough. A busy wait loop for 0.1 seconds should easily get the 5% precision required by the assertAlmostEqual. Added file: http://bugs.python.org/file9515/test_posix2.PATCH _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: I was wrong -- 0.1 isn't enough, because os.times() typically has 0.01s resolution, so we can easily get 0.1 vs. 0.11 which will fail the assertion. Cranked up the WAIT_TIME to 0.3 in the attached patch (test_posix3.PATCH). Sorry for the noise. Added file: http://bugs.python.org/file9516/test_posix3.PATCH _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Sorry, but the test was still wrong because I misunderstood how assertAlmostEqual works. Attaching a fourth (final?) test. Added file: http://bugs.python.org/file9517/test_posix4.PATCH _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Alexander, regarding your comments: 1. sysconf in general returns a long because it can return all sorts of information, but os.times() returns clock_t items, so the _SC_CLK_TCK value must comfortably fit into a clock_t. It's preferable to cast into a clock_t immediately rather than doing a conversion for each of the ensuing divisions. 2. Do you have indications that such platforms exist? In that case, indeed the patch should be adapted. Is that -1 return value documented somewhere? 3. I agree; 0 or -1 would be better. 4. You're right about the overhead, but someone (amk?) measured it and it's only 5% compared to the old buggy behaviour. It's still possible to do a million calls to os.times() from Python in a second, which is plenty fast enough. Clearly the speed could be improved, but it doesn't appear worth the complications to me. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Alexander, speed-wise your patch is worse than the original one on systems where HZ isn't predefined, because it calls sysconf 5 times in each call to os.times, rather than only once per call. If you worry about speed, the approach outlined in Antoine's last message makes most sense to me. Doing this in the configure script appears dangerous to me; it is well possible that the clock tick value changes over time on the same machine (e.g. after a kernel upgrade), so this should be determined upon process initialization, not before compilation. Also, I don't think that the HZ value should be preferred to the sysconf value if both are available, as all times man pages I could check mention sysconf as the correct way to do this, not HZ. (Some of them state that HZ is used on "older systems".) Finally, your patch assumes that HAVE_TIMES implies HAVE_SYSCONF; is that guaranteed? In particular, it's not clear to me what happens on Windows (see comment at the top of the file). I also have no idea how any of the earlier patches behaves on Windows, unfortunately. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Tiran, that's the general approach we should follow, yes. But the people who discussed this on #python-dev all felt a bit queasy about the "60" fallback -- this is what caused the bug in the first place on Guido's and my machine. (A value of 60 was assumed; 100 would have been correct.) Having no such fallback would be preferable, unless it's necessary. You use Windows, right? Can you test if that fallback is necessary there? As far as I can tell, it should not be necessary on a more UNIX-ish system. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Never mind, on Windows a different code path is chosen. I'm now working on a patch that computes the hz value during module initialization. So should I keep the 60 magic value? What is the justification? _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Here is an updated patch (os_times4.PATCH) that incorporates Christian's suggestions. The patch includes the new unit test, so test_posix?.PATCH need not be applied separately. I again made the unit test a bit more lenient to allow an absolute error of 0.02 seconds, as there may be systems where the clock tick granularity is only 1/60 seconds, and then the old tolerance of 0.015 seconds would be too low. This patch prefers sysconf where it is available; this is what "man 2 times" asks to do. If sysconf is available but produces an error, that error is raised. (Errors should never pass silently.) HZ is only used if sysconf is not available. If neither sysconf nor HZ is available, a compile-time error is raised -- in that case, HAVE_TIMES shouldn't have been defined in the first place. I also timed this; there is no discernible change compared to the old behaviour. The patch fixes the buggy behaviour on my 64-bit Linux box and makes no difference on my 32-bit Linux box. The new unit test passes on both machines. Added file: http://bugs.python.org/file9540/os_times4.PATCH _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: I'd prefer a noisy compile error, since in situations where times is available but unusable, HAVE_TIMES shouldn't have been #defined in the first place. (That is, I'd see that as a bug in the configure script.) But this is turning into a bikeshed discussion. I care more about the bug being fixed than about how precisely it is fixed. For the record, Alexander's patch fixes the bug on my Linux box, too, so I'm fine with either patch. If Alexander's patch ends up being applied, I suggest ripping the new unit test from os_times4.PATCH, since the timing in test_posix4.PATCH is fragile as mentioned above. Either way, this looks ready to be closed to me. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: >> I'd prefer a noisy compile error .. > > That would be fine if you could verify that none of the currently > supported platforms will be affected. I would still feel uneasy about > refusing to build python simply because os.times is not ported to a > platform. Unless I'm missing something, your suggested one-line change fails to compile in exactly the same cases -- if HAVE_TIMES is defined, but HZ and sysconf unavailable -- but with a worse error message. > HAVE_TIMES shouldn't have been #defined in the > first place. (That is, I'd see that as a bug in > the configure script.) > No, defined HAVE_TIMES only tell you that the system has 'times' > function in the C library. It is not intended to mean that os.times > is implementable. Sure, but if times is in the standard library, but its output is uninterpretable, then there's something wrong going on that needs to be fixed rather than swept under the rug. > Personally, I would still prefer a one-line change that I proposed > above. It is obviously better than the current smiley code and if it > happens to fix the platforms where errant behavior was observed, it is > worth applying even if theoretically it may be wrong. You complained in msg62869 about the original patch that calling sysconf on every call leads to an unacceptable slowdown. Your one-line patch calls sysconf five times on each call when HZ is not defined. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Alexander, your one-line patch *does* affect performance on my 64-bit Linux machine in a worse way than any other proposed patch by calling sysconf five times. HZ may be defined on your machine, but it isn't on my (Xeon) machine. I checked man pages on four different Linuxes (32 bit and 64 bit; SuSE, Fedora, Ubuntu; recent or six years old). All of them state that using the sysconf value is the right thing to do. This is also stated in the man page excerpt in Guido's original bug report. Neither your latest patch (posixmodule.diff) not my latest patch (os_times4.PATCH) affects performance; they both only call sysconf once and then used a cached value. I'm perfectly fine with your posixmodule.diff, which also meets Antoine's criteria. I suggest we apply that patch, along with the unit test from os_times4.PATCH, and be done with it. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Antoine, none of the recently proposed patches uses the 60 magic value. Alexander's patch doesn't define times in that case (leading to an AttributeError when trying to call os.times) while my patch complains about the bogus environment during compilation. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: s/standard library/system library/, of course. Also, the original code is wrong in preferring HZ over the sysconf value. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: If I remove the "#define 60" line in an unmodified posixmodule.c from trunk, I get the following compiler error: gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -I. -IInclude -I./Include -DPy_BUILD_CORE -c ./Modules/posixmodule.c -o Modules/posixmodule.o ./Modules/posixmodule.c: In function posix_times: ./Modules/posixmodule.c:5964: error: HZ undeclared (first use in this function) So yes, HZ isn't available there. It sure is defined *somewhere* (grep shows a definition in /usr/include/asm-x86_64/param.h), but it isn't anywhere Python would pick it up. And I don't really see why it should when the man pages all argue that using HZ for times is for "older system" (this is a man page from 2002, no less!). Can you measure a performance difference between your patch and the old buggy behaviour? I couldn't, on any machine, and I'd be very surprised if it existed. There is no significant difference between dividing by a constant and diving by a static module variable, and, as Antoine rightly suggests, any such difference would be completely lost in the noise considering the hefty cost of the other operations. Regarding your two issues: 1. Yes, the sysconf value should take precedence over HZ, since this is what "man 2 times" says you should use. 2. Personally, I'd be just as fine with the original patch that doesn't have the code complexity of the value caching, but I'm fine with anything that fixes the bug. Keeping the old behaviour where possible "for old time's sake" seems a bad idea -- there were at least two broken platforms (Mac OS and Xeon), and there may be others. Using the documented behaviour (sysconf) where available is a much better solution; honestly, sticking to using HZ as a default without support for that from any documentation has a cargo-cult programming smell to me. I don't know if there are platforms that have times, but neither sysconf nor HZ. That sounds very strange to me, but of course I can't rule it out. But if there are, it is likely that os.times was broken before on these platforms -- it was broken on two platforms that I wouldn't consider minor. In that case, it will still be broken, but at least now we have a unit tests that detects this. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: In the first line of my previous message, I mean "#define HZ 60", of course. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Christian, I agree on all points. Alexander's patch posixmodule.diff satisfies those requirements. I suggest also adding the unit test from os_times4.PATCH (but not the changes to posixmodule.c in that patch), as this will help identify misbehaving platforms in the future. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: Great, we're approaching closure! :-) One final small thing: As said somewhere above, the allowed discrepancy in test_posix4.PATCH is a bit too low for machines with only 60 ticks per second. I uploaded a slightly more generous test_posix5.PATCH instead. So posixmodule.diff + test_posix5.PATCH. This is the same as what I mentioned above (posixmodule.diff + only the unit test from os_times4.PATCH). Added file: http://bugs.python.org/file9542/test_posix5.PATCH _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: I think it's better only to only add another fallback if the unit tests show that such platforms exist. Avoiding cruft is important, too. After all, sysconf is a standard POSIX API, and from my (admittedly limited) research was already available in that form back in 1988. So my suggestion is to apply the current patches. _ Tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> _ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Changes by Malte Helmert <[EMAIL PROTECTED]>: Removed file: http://bugs.python.org/file9540/os_times4.PATCH ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert <[EMAIL PROTECTED]> added the comment: Martin, compilation indeed breaks if sysconf is available but _SC_CLK_TCK is not. My Unix-foo is not sufficient to confidently say that this is impossible, so my suggestion is to add defined(_SC_CLK_TCK) to the condition of this #ifdef branch. For what it's worth, this also appears to be the way Perl does it (perl.c, lines 384-385): #if defined(HAS_SYSCONF) && defined(_SC_CLK_TCK) && !defined(__BEOS__) PL_clocktick = sysconf(_SC_CLK_TCK); In the other case you mention, where neither sysconf nor HZ are available, the old default of 60 could be used instead. A noisy error appears safer to me to avoid future similar bugs, but I see that this is a bad idea for Python 2.5.x. I'll prepare a modified patch. ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert <[EMAIL PROTECTED]> added the comment: OK, modified and simplified patch attached (os_times5.PATCH). The patch and unit test (in test_posix5.PATCH) apply cleanly against the trunk. "make test" passes on two machines I tried, including a 64-bit Linux machine where the new unit test fails without the patch. The caveat is that with the machines I have, I can't exercise all #ifdef paths. However, the logic looks simple enough now. Added file: http://bugs.python.org/file11826/os_times5.PATCH ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1040026> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1040026] os.times() is bogus
Malte Helmert added the comment: > I see. I wish that people would a) always provide complete patches in > a single file, and b) delete files themselves that have been > superceded by others. In any case, I have re-attached the file; > thanks for pointing this out. Regarding b), I did that as far as I could (i.e., deleted my own files that were superseded). I left the unit test as a separate file because I wasn't too sure if it would get incorporated -- it looks a bit flaky because it relies on timing issues, and it's also one of the longer-running tests as it spends (comparatively) a lot of time in a busy loop. I wouldn't be surprised if it breaks on some machines sometimes, especially ones with low clock granularity. Thanks for the comment though, I'll keep it in mind in the future. ___ Python tracker <http://bugs.python.org/issue1040026> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue2170] rewrite of minidom.Node.normalize
Malte Helmert added the comment: I eyeballed the new patch and wonder if the case where text nodes are collapsed is handled correctly. Assume that self.childNodes contains nodes [A, B, C], where A and B are non-empty text nodes and C is a non-text node. The algorithm would collapse A and B into A and then unlink B, and I think C.previousSibling would still reference the unlinked B, rather than the correct A. Am I missing something? If this is indeed a bug in the proposed patch, I suggest adding an additional test for this case. The bug itself should not be too hard to fix; the "elif" case would need the same if child.nextSibling: child.nextSibling.previousSibling = child.previousSibling assignment as the "if" case. -- ___ Python tracker <http://bugs.python.org/issue2170> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue2170] rewrite of minidom.Node.normalize
Malte Helmert added the comment: While we're cleaning up: data = child.data if not data: could be if not child.data: since data is not used again. Alternatively, you could use data in place of child.data later on for a small speed-up, but I doubt that we care about such small optimizations here. If we do, then we should also assign a name to child.nextSibling, though. I found the three-line duplication between the two branches mildly refactor-worthy, but when I eliminated it the control logic got more involved and I don't think that's a good trade-off here. -- ___ Python tracker <http://bugs.python.org/issue2170> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue2170] rewrite of minidom.Node.normalize
Changes by Malte Helmert : Removed file: http://bugs.python.org/file9513/minidom.diff ___ Python tracker <http://bugs.python.org/issue2170> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue2170] rewrite of minidom.Node.normalize
Malte Helmert added the comment: I removed my original patch which has been superseded by David's patch. David, I suggest that you also remove test_minidom.patch which is superseded by your later patch (see http://bugs.python.org/msg77766 for policy on this). -- ___ Python tracker <http://bugs.python.org/issue2170> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue2170] rewrite of minidom.Node.normalize
Malte Helmert added the comment: Short review: code looks good to me, patch applies cleanly to trunk, passes tests. @akuchling: I don't know if you remember, but this rewrite was originally suggested by you on a bug day some time ago. I think David's patch is ready to be applied. -- ___ Python tracker <http://bugs.python.org/issue2170> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
