[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Kyle Stanley added the comment: > Is there an "aesthetic code cleanup" patch that's ever turned out well? ;-) >From what I can tell, any remotely extensive aesthetic code patch I've seen >has been highly controversial. I think they can have value in some cases, but >the value relative to

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Kyle Stanley added the comment: > You can find my email in Git, and I'm on Zulip and Discourse; and I'd be > happy to start or follow a thread in a forum you think appropriate. Or if > you'd rather drop it entirely, that's fine too. I think opening a thread in

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Greg Price added the comment: > labeling long-stable code as "evil". Let me apologize about this bit -- I was thinking of the term in quotes (referring to the earlier comment), but I didn't write it clearly that way. I don't think any of this code is evil, past or present, and I regret

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Tim Peters added the comment: Sorry, but there was nothing wrong with the CHECK_SMALL_INT macro, to my eyes, to begin with - except that it was burdened with an over-elaborate "do ... while(0)" wrapper. Not all macros are _intended_ to be "cheap functions". Like this one, some are

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Ma Lin added the comment: Recent commits for longobject.c Revision: 5e63ab05f114987478a21612d918a1c0276fe9d2 Author: Greg Price Date: 19-8-25 1:19:37 Message: bpo-37812: Convert CHECK_SMALL_INT macro to a function so the return is explicit. (GH-15216) The concern for

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Raymond Hettinger added the comment: I'm unassigning this. It has been blown profoundly out of proportion. The recommendation of the senior most developer is being ignored, and now two developers are speaking in terms of strong belief systems and labeling long-stable code as "evil". This

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Greg Price added the comment: I hesitate to come back to this thread, because as Raymond says it's consumed a lot of time already. But I think this point is of broader interest than the specific few lines we've been discussing: > When small integer are disabled at compilation time

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Kyle Stanley added the comment: > I'm one of the first to advocate to replace ugly macros with clean static > inline functions. Macros are evil and can be too easily misused. As someone who has only more recently started learning the C-API (and C in general), I'm certainly in favor of

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

STINNER Victor added the comment: When small integer are disabled at compilation time (NSMALLPOSINTS=0 and a NSMALLNEGINTS=0), I suggest to remove code related to small integers. IMHO CHECK_SMALL_INT() macro is a good practical solution for that. So I suggest to restore this macro.

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

STINNER Victor added the comment: Raymond: > We're wasted a lot of dev time on something that never promised much value in > the first place. I'm one of the first to advocate to replace ugly macros with clean static inline functions. Macros are evil and can be too easily misused. On PR

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Raymond Hettinger added the comment: Greg, would you be willing to work on a PR that reverts pr15216 and pr15710 while preserving the efforts of pr15192 ? -- ___ Python tracker

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Greg Price added the comment: > We're wasted a lot of dev time on something that never promised much value in > the first place. So, I'll revert and close this tracker issue OK, so be it. I'll certainly agree that this series of threads consumed a lot more time than I anticipated or

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Ma Lin added the comment: > I agree that both changes should be reverted. There is another commit after the two commits: https://github.com/python/cpython/commit/c6734ee7c55add5fdc2c821729ed5f67e237a096 It is troublesome to revert them. PR 16146 is on-going, maybe we can request the author

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Greg Price added the comment: > if using a static inline function is causing issues Separately from whether there was or wasn't such an issue here, I think it's interesting to note that the build failure bpo-38205 is an example of exactly the opposite! It was caused by a combination of (a)

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Raymond Hettinger added the comment: Sorry Greg, I know you're enthusiastic about this but I think both changes were a mistake. As the person who merged them, I've decided to revert them in favor of stable code (FWIW, I do care about Ma Lin's concerns in the other BPO and I don't want the

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Ma Lin added the comment: > It's not clear to me if anyone benchmarked to see if the > conversion to a macro had any measurable performance benefit. I tested on that day, also use this command: python.exe -m pyperf timeit -s "from collections import deque; consume = deque(maxlen=0).extend;

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Greg Price added the comment: Thanks Victor for linking that issue back here. > A first change converted a macro to a static inline function. The second > change converted the static inline fnuction to a macro Not quite. The first change converted a macro `CHECK_SMALL_INT` to an equivalent

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Raymond Hettinger added the comment: Victor, I agree that both changes should be reverted. We should avoid churning long stable, bug free code for minimal aesthetic value. -- ___ Python tracker

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

STINNER Victor added the comment: Sadly, GH-15710 was merged without mentioning the bpo-37812 (this issue) in the final commit message :-( commit 6b519985d23bd0f0bd072b5d5d5f2c60a81a19f2 Author: animalize Date: Fri Sep 6 14:00:56 2019 +0800 replace inline function `is_small_int` with

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Greg Price added the comment: > May I suggest directing your efforts towards fixing known bugs or > implementing requested features. Well, I would certainly be grateful for a review on my fix to #18236. ;-) There's also a small docs bug at GH-15301. I do think there's significant value in

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Kyle Stanley added the comment: > May I suggest directing your efforts towards fixing known bugs or > implementing requested features. It isn't our goal to create more work for > one another There frequently is value in improving code readability, as it can improve maintainability in the

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Raymond Hettinger added the comment: New changeset 5e63ab05f114987478a21612d918a1c0276fe9d2 by Raymond Hettinger (Greg Price) in branch 'master': bpo-37812: Convert CHECK_SMALL_INT macro to a function so the return is explicit. (GH-15216)

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Change by Raymond Hettinger : -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker ___

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Raymond Hettinger added the comment: > I noticed a very similar story in CHECK_BINOP. How about we don't do any more of these :-) I understand the desire to have an explicit return but don't think these minor aesthetics are worth it. The existing code was correct. The patches increase the

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Change by Kyle Stanley : -- type: -> enhancement ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Kyle Stanley added the comment: > python-dev likely isn't the right place. That is a forum for more mature > ideas. Agreed, that idea should be posted to python-list if they're having issues posting to python-ideas. Python-dev certainly wouldn't be appropriate for that. --

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Greg Price added the comment: Thanks, Raymond, for the review on GH-15216! Shortly after posting this issue, I noticed a very similar story in CHECK_BINOP. I've just posted GH-15448 to similarly make returns explicit there. It basically consists of a number of repetitions of -

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Change by Greg Price : -- pull_requests: +15140 pull_request: https://github.com/python/cpython/pull/15448 ___ Python tracker ___

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Raymond Hettinger added the comment: python-dev likely isn't the right place. That is a forum for more mature ideas. -- ___ Python tracker ___

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Ma Lin added the comment: I sent twice, but it doesn't appear in Python-Ideas list. I will try to post to Python-Dev tomorrow. -- ___ Python tracker ___

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Raymond Hettinger added the comment: Ma Lin: Everything in Python is an object, nothing is native. There is room to reconsider the implementation of integer objects, known internally as PyLong objects. If you want to tease-out an alternative implementation, please do so on the

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Ma Lin added the comment: How about using a hybrid implementation for `int`. For example, on 64-bit platform, if (a integer >=-9223372036854775808 and <=9223372036854775807), use a native `signed long` to represent it. People mostly use +-* operations, maybe using native int is faster, even

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Greg Price added the comment: > On the other hand, if it is tricky and requires something more than minor > surgery, that would be a hint that it isn't worth it. There is some value in > code that is stable and known to be working just fine. Definitely true! I think this change turned out

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Change by Greg Price : -- pull_requests: +14944 pull_request: https://github.com/python/cpython/pull/15216 ___ Python tracker ___

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Raymond Hettinger added the comment: > I'll rework the explicit-return patch to send without this > change. It'll be a little trickier but should be entirely doable. An explicit return would be a nice improvement. On the other hand, if it is tricky and requires something more than minor

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Greg Price added the comment: > Sometimes for testing we turn it off in order to identify identity test bugs. Interesting! Well, if the option is useful for testing, that's certainly a good reason to keep it. > Also, eveonline was controlling this to save memory. Also interesting. I feel

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Raymond Hettinger added the comment: P.S. Making the returns explicit is a reasonable change to make, but the core #ifdef logic should remain. -- ___ Python tracker ___

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Raymond Hettinger added the comment: Thanks for the contribution Greg. I understand your reasoning but am going to decline PR 15203. Though the feature is not experimentnal, it is something we want be able to modify, shut-off, or extend at build time. Sometimes for testing we turn it off

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Greg Price added the comment: I've just sent GH-15203 which is the first of two patches for this. It's quite small. The second patch, which completes the change, is also small: https://github.com/gnprice/cpython/commit/c6b905104 It depends on the first one, so I think the easiest is for me

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

Change by Greg Price : -- keywords: +patch pull_requests: +14932 stage: -> patch review pull_request: https://github.com/python/cpython/pull/15203 ___ Python tracker ___

[issue37812] Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

New submission from Greg Price : In longobject.c we have the following usage a few times: PyObject * PyLong_FromLong(long ival) { PyLongObject *v; // ... more locals ... CHECK_SMALL_INT(ival); if (ival < 0) { /* negate: can't write this as abs_ival = -ival since that