| msg272119 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
Date: 2016-08-07 12:51 |
| In do_raise, the two Py_XDECREFs at the final of the success branch seem doing unnecessary NULL checks. |
|
|
| msg272134 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2016-08-07 22:28 |
| Have you checked all the possibly code paths to be sure? "Seems to be unnecessary" is insufficient reason for undoing Guido's code that has stood since 1997. Looking at the snarl of possible code paths, I not finding it obvious why some of the paths require Py_XDECREF and others don't. Even if you are sure, then the patch needs to include assertions at key checkpoints (i.e. after a given assignment to "type" or "value" can we reliably assert the value is non-null) and/or comments showing what the reasoning is. Otherwise, even if the patch happens to be correct, it increases the risk that future maintenance will introduce a bug when the current code would have allowed the maintenance in a safer context. |
|
|
| msg272143 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2016-08-08 03:05 |
| Thanks for your reply Raymond. You always provide thoughtful feedbacks, though it makes me somewhat more nervous. ;) (I am nervous about making noise here and waste others' time.) > Have you checked all the possibly code paths to be sure? Yes. I did check all the code paths. I don't want to be a noise maker so I did check it carefully. > "Seems to be unnecessary" is insufficient reason for undoing Guido's code that has stood since 1997. I use the word "seems" because even if I think my opinion is right, I am only 99% sure. Just as you said, the code is legacy and changing it has to be careful. I can miss something and be wrong. But now I think I am right. > Looking at the snarl of possible code paths, I not finding it obvious why some of the paths require Py_XDECREF and others don't. When we reach the two Py_XDECREFs, can type and value be NULL? > then the patch needs to include assertions at key checkpoints (i.e. after a given assignment to "type" or "value" can we reliably assert the value is non-null) and/or comments showing what the reasoning is Sorry, I didn't realize that. This is definitely good practice I know. But this is not documented and not a must then so I didn't do that. But I'll remember this in later patches. |
|
|
| msg272145 - (view) |
Author: Antti Haapala (ztane) * |
Date: 2016-08-08 06:20 |
| Normally I wouldn't recommend changing working code. However those asserts would be OK; if either of them is NULL, then the previous if would have had undefined behaviour already. Thus the `XDECREF` wrongly signals that it'd be OK if they were NULLs until this point, which is not true. I'd rather see more asserts in the code; would be a big aid in possible refactoring; now for example `PyErr_SetObject` checks twice and thrice if either of the arguments is NULL; would be nice to go see the call site and see asserts in place there, showing that the arguments never were NULL to begin with. |
|
|
| msg272148 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2016-08-08 06:50 |
| Thanks for the comment, Antti. > would be nice to go see the call site and see asserts in place there, showing that the arguments never were NULL to begin with. So is it still not enough getting the two asserts? IMHO I think the two asserts have already meet your expectations since the cause block is impossible to make type or value NULL. |
|
|
| msg272213 - (view) |
Author: Antti Haapala (ztane) * |
Date: 2016-08-09 04:05 |
| No, I was just trying to explain why your change could be considered beneficial. |
|
|
| msg272226 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-08-09 09:40 |
| do_raise_v2.patch (with asserts) LGTM. There are two ways that lead to this point (and third way leads to raising an exception), and additional assertions make the code clearer, because a reader shouldn't follow all these patch for reading the code after assertions. |
|
|
| msg277506 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2016-09-27 08:38 |
| New changeset 9e59cb403efa by Serhiy Storchaka in branch 'default': Issue #27703: Got rid of unnecessary NULL checks in do_raise() in release mode. https://hg.python.org/cpython/rev/9e59cb403efa |
|
|