| msg209663 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2014-01-29 18:14 |
| Proposed patches replaced idiomatic code Py_DECREF(ptr); ptr = new_value; to "Py_REPLACE(ptr, new_value);" which is expanded to { PyObject *__tmp__ = ptr; ptr = new_value; Py_DECREF(__tmp__); } (and same for Py_XDECREF -> Py_XREPLACE). Victor proposed large patch for , but this issue was closed after fixing particular bug. Here are updated patches, which Py_REPLACE/Py_XREPLACE macros for cleaner code. They are also generated automatically by the Coccinelle tool (http://coccinelle.lip6.fr/): spatch --in-place --sp-file py_replace.spatch --dir . Patch for every version contains about 50 replaces in about 21-24 files. |
|
|
| msg209666 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2014-01-29 18:35 |
| Something similar was proposed in . |
|
|
| msg209669 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-01-29 19:00 |
| I do not understand why that issue was closed. Probably Py_SETREF is not the best name but this can be discussed. Adverse idea about Py_INCREF also looked questionable. But many people supported the introduction of a macro for safe replacement. And now we see that sources contain 50 potential bugs which can be fixed either by using special macros or by manually inlining them. |
|
|
| msg209670 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2014-01-29 20:00 |
| It all seems like a good idea to me and I like the Py_REPLACE naming (I was going to suggest Py_SWAP, but Py_XSWAP looks too weird to me). |
|
|
| msg209701 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2014-01-30 04:28 |
| Unless some known bugs are being fixed, this should be 3.4 only. |
|
|
| msg209713 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-01-30 12:58 |
| > Unless some known bugs are being fixed, this should be 3.4 only. For example here is a bug very similar to a bug fixed in . class Nasty(str): def __del__(self): C.__qualname__ = "other" class C: pass C.__qualname__ = Nasty("abc") C.__qualname__ = "normal" C.__qualname__ = "normal" |
|
|
| msg209894 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2014-02-01 16:06 |
| I think Raymond's original concern still applies: The macros do add to the learning curve. I would personally expect that Py_REPLACE(op, op2) does an INCREF on op2, but it does not. Explicit is better than implicit. |
|
|
| msg209984 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-02-02 13:53 |
| > I think Raymond's original concern still applies: The macros do add to the learning curve. I agree. But alternative solution is Victor's original patch which replaces potential bugs by inlined body of Py_REPLACE/Py_XREPLACE. This is explicit, but more verbose (2 lines are replaced by 5 lines with one new variable, with macros it would be one line), less clear and more errorprone. I believe that given the popularity of such a code and the possibility of mistakes, it is worth introducing special macros. Here apply the same reasoning as for Py_CLEAR. Of course these macros shouldn't be a part of stable API in 2.7 and 3.3 (and may be even in 3.4). |
|
|
| msg210447 - (view) |
Author: Kristján Valur Jónsson (kristjan.jonsson) * |
Date: 2014-02-07 10:50 |
| These macros work as assignment with builtin decref, i.e. a smart replacement for = We could resolve this by calling them Py_ASSIGN Py_XASSIGN and having complementary macros Py_STORE/Py_XSTORE that will incref the new value. However, with an added incref, does the X apply to the source or the target? I wonder if we need the X variants in these macros. Once you are doing things like this, why not just use X implicitly? An extra pointer test or two is unlikely to be a performance problem in the places you might use them. Anyway, I'll be adding this to the internal api of stackless because it is tremendously useful. |
|
|
| msg210567 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-02-07 21:51 |
| Py_ASSIGN was proposed by Paul Pogonyshev in , and this also looks good to me. |
|
|
| msg212217 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-02-25 21:44 |
| While we're bikeshedding, how about the more verbose PY_DECREF_AND_ASSIGN? That makes it clearer that an INCREF is not done. Regarding Kristján's suggestion of PY_ASSIGN and a complementary PY_STORE, IMO these names are too similar and the difference between them is not clear. |
|
|
| msg212219 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-02-25 21:52 |
| > While we're bikeshedding, how about the more verbose PY_DECREF_AND_ASSIGN? That makes it clearer that an INCREF is not done. Py_ASSIGN_AND_DECREF would be more correct. And Py_CLEAR can be renamed to Py_CLEAR_AND_XDECREF or Py_ASSIGN_NULL_AND_XDECREF. |
|
|
| msg212223 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-02-25 22:00 |
| PY_ASSIGN_AND_DECREF could seem to imply that the assigned value is DECREF-ed. I think PY_DECREF_AND_ASSIGN makes it clearer that the original value is DECREF-ed. I like PY_ASSIGN_NULL_AND_DECREF, though for the same reason as above, I'd name it PY_DECREF_AND_ASSIGN_NULL. |
|
|
| msg212230 - (view) |
Author: Kristján Valur Jónsson (kristjan.jonsson) * |
Date: 2014-02-26 00:06 |
| Better yet, embrace c++ and smart pointers :;-) |
|
|
| msg212246 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-02-26 10:17 |
| Poll: http://comments.gmane.org/gmane.comp.python.devel/145974 |
|
|
| msg212278 - (view) |
Author: Kristján Valur Jónsson (kristjan.jonsson) * |
Date: 2014-02-26 17:29 |
| Barring c++, are we using any C compilers that don't support inlines? Imho these macros should be functions proper. Then we could do Py_Assign(&target, Py_IncRef(obj)) It's 2014 already. |
|
|
| msg212279 - (view) |
Author: Stefan Krah (skrah) * |
Date: 2014-02-26 17:52 |
| > Barring c++, are we using any C compilers that don't support inlines? Not that I know of. libmpdec is C99, which seems to be supported by all obscure commercial compilers on snakebite. Also there have been no 3.x bug reports due to compilers choking on inline functions. |
|
|
| msg212283 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-02-26 18:23 |
| > Barring c++, are we using any C compilers that don't support inlines? CPython advertises itself as C89 compliant, and C89 doesn't have inlines. You need to go to C99 to get inlines. And before you ask--yes, we support a compiler that is not C99 compliant: Microsoft Visual C++. I'm pretty sure it does have inline support though. It's possible that every platform officially supported by CPython has a C compiler that supports inlines. I'm pretty sure people compile Python on unsupported platforms whose compilers don't have inlines (e.g. OS/2). Anyway, you'd have to get Guido to agree to breaking C89 compatibility, it's not something you could do locally on this patch without (most likely) a big drawn-out discussion on python-dev. |
|
|
| msg212346 - (view) |
Author: Kristján Valur Jónsson (kristjan.jonsson) * |
Date: 2014-02-27 09:56 |
| Are you referring to the Py_LOCAL_INLINE macro? I see that we have no Py_INLINE. Py_LOCAL_INLINE includes the "static" qualifier, and in fact, if there is no "USE_INLINE" defined, then all that it does is to add "static". Would having a "Py_INLINE(type)" macro, that is the same, but without the static (except when USE_INLINE is false) make a difference? It would be a bit odd to have Py_LOCAL_INLINE() functions defined in the headers. I'm not sure that there is any practical difference between "static inline" and "inline". But there is a difference between "static" and "inline". It would be great if we could start writing stuff like the Py_INCREF() and Py_DECREF() as functions rather than macros, but for this to happen we must be able to trust that they are really inlined. |
|
|
| msg212347 - (view) |
Author: Kristján Valur Jónsson (kristjan.jonsson) * |
Date: 2014-02-27 10:07 |
| Well, Larry, I certainly am in no mood to start wrangling on python-dev. A 25 year old C standard is likely to be very mature and reliable by now. Why take risks? :) #Py_LOCAL_INLINE exists and demonstrates that we can make use of them when possible. We could create a #Py_INLINE macro that would work the same, only not necessarily yield inline on some of the older compilers. It would really be healthy for the pyton code base, for quality, for semantics, and for the learning curve, if we could start to rely less on macros in the core. Ah well, perhaps I'll throw this out there... |
|
|
| msg213980 - (view) |
Author: Jim Jewett (Jim.Jewett) *  |
Date: 2014-03-18 15:29 |
| I am changing this from "High" to "Release blocker", because I think 3.3 should make an explicit decision about whether to remove itself from the Affected Versions list. 3.4 probably should too, since it is now in bug-fix mode. Then this issue can go back to whatever level is otherwise appropriate. |
|
|
| msg213983 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2014-03-18 15:48 |
| "I am changing this from "High" to "Release blocker", because I think 3.3 should make an explicit decision about whether to remove itself from the Affected Versions list." I don't understand. Which release does it block? There is no scheduled release in short term. Python 3.3 now only accept security changes, and so it's too late for such change. |
|
|
| msg213986 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2014-03-18 16:39 |
| The patch adds new public APIs (C macros), I don't think it should be committed to the maintenance branches. |
|
|
| msg213989 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-03-18 16:50 |
| Yeah, I'm not accepting this for 3.4 at this point, and I bet the other RMs feel the same way. |
|
|
| msg213993 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2014-03-18 17:42 |
| Yes, this is new feature territory. |
|
|
| msg256802 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-12-21 14:54 |
| According to the results of recent poll [1], Py_SETREF is the most popular candidate. It was also one of leaders in previous poll [2], and this is a name used in original Antoine's proposition [3]. [1] http://comments.gmane.org/gmane.comp.python.devel/155654 [2] http://comments.gmane.org/gmane.comp.python.devel/145974 [3] https://mail.python.org/pipermail/python-dev/2008-May/079862.html Hence there is a patch that introduces and uses the Py_SETREF macro. |
|
|
| msg256956 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2015-12-24 08:40 |
| New changeset 23296440b654 by Serhiy Storchaka in branch '2.7': Issue #20440: Massive replacing unsafe attribute setting code with special https://hg.python.org/cpython/rev/23296440b654 New changeset fd36d72f6030 by Serhiy Storchaka in branch '3.5': Issue #20440: Massive replacing unsafe attribute setting code with special https://hg.python.org/cpython/rev/fd36d72f6030 New changeset c4e8751ce637 by Serhiy Storchaka in branch 'default': Issue #20440: Massive replacing unsafe attribute setting code with special https://hg.python.org/cpython/rev/c4e8751ce637 |
|
|
| msg256957 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-12-24 08:58 |
| Committed patches were generated with attached Coccinelle script. |
|
|
| msg256958 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-12-24 09:04 |
| Following patch is manually crafted and covers the rest cases. It also replaces existing correct attribute replacing using a temporary variable with more compact call of the macro. |
|
|
| msg257069 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2015-12-27 10:44 |
| New changeset 9fb57c0209ea by Serhiy Storchaka in branch '3.5': Issue #20440: Applied yet one patch for using Py_SETREF. https://hg.python.org/cpython/rev/9fb57c0209ea New changeset bc7c56a225de by Serhiy Storchaka in branch 'default': Issue #20440: Applied yet one patch for using Py_SETREF. https://hg.python.org/cpython/rev/bc7c56a225de New changeset e6502bf289ab by Serhiy Storchaka in branch '2.7': Issue #20440: Applied yet one patch for using Py_SETREF. https://hg.python.org/cpython/rev/e6502bf289ab |
|
|
| msg257075 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2015-12-27 13:52 |
| New changeset 4bfbb2714ae9 by Serhiy Storchaka in branch '3.5': Issue #20440: More use of Py_SETREF. https://hg.python.org/cpython/rev/4bfbb2714ae9 New changeset 11670e4be1a9 by Serhiy Storchaka in branch '2.7': Issue #20440: More use of Py_SETREF. https://hg.python.org/cpython/rev/11670e4be1a9 New changeset 539ba7267701 by Serhiy Storchaka in branch 'default': Issue #20440: More use of Py_SETREF. https://hg.python.org/cpython/rev/539ba7267701 New changeset b02d256b8827 by Serhiy Storchaka in branch 'default': Issue #20440: Cleaning up the code by using Py_SETREF and Py_CLEAR. https://hg.python.org/cpython/rev/b02d256b8827 |
|
|
| msg257164 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2015-12-29 05:51 |
| New changeset e04cd497aa41 by Zachary Ware in branch 'default': Issue #25972, #20440: Fix compilation on Windows https://hg.python.org/cpython/rev/e04cd497aa41 |
|
|
| msg257527 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-01-05 11:29 |
| The final patch replaces the code that equivalent to the Py_SETREF macro by using the Py_SETREF macro. There are no bugs, the patch only makes the correct code cleaner. If I'll not got a review, I'll just close this issue. |
|
|
| msg257539 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2016-01-05 19:28 |
| New changeset 1118dfcbcc35 by Serhiy Storchaka in branch 'default': Issue #20440: Cleaning up the code by using Py_SETREF. https://hg.python.org/cpython/rev/1118dfcbcc35 |
|
|
| msg257540 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-01-05 19:34 |
| The commit doesn't include changes for dictobject.c, setobject.c and _sqlite/cache.c (I had forgot to exclude them from the patch before uploading). Dict and set code is performance critical, and using Py_XDECREF instead of Py_DECREF can affect performance. The code in _sqlite would use Py_SETREF in less obvious way, it is better to left it as is. |
|
|