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

[Kyle Stanley]

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 the potential cost may have been a bit overstated in 
>this particular case.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

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 https://discuss.python.org/c/users to talk about 
deciding between the usage of functions and macros (discussing when each may be 
appropriate) would be beneficial to the community at large.

> 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 doesn't bode well and it makes it difficult 
> to conduct reasoned discourse.  

Apologies if I added to that, I certainly respect your judgement on this issue. 
Pretty much everyone involved in this discussion has more experience in working 
with the CPython C-API than I do, you most of all (as far I'm aware). My 
perspective was coming from someone attempting to understand it better, and 
explaining how those learning the C-API might find it confusing.

I don't find the code itself to be at all "evil", just potentially confusing. 
That long-stable code has provided a lot of benefit over the years, and I can 
definitely appreciate the effort that was put into writing it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

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 that I 
carelessly gave that impression.

I think there isn't currently any problem to be solved here, and I hope we can 
all put our energy elsewhere.  (For my part, I just spent the balance of the 
evening implementing an old feature request on unicodedata.)


Victor, I'd still be quite interested in better understanding your thinking (as 
this example relates to macros in general) if you'd like to reply, though this 
thread may not be the right place to continue.  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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Tim Peters]

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 
intended to be merely textual replacement, to minimize tedious and error-prone 
redundant typing, in a specific file.  Even "bulletproofing them" with tricks 
like "do ... while(0)" cruft is counterproductive in such cases.  They're not 
_intended_ to capture abstractions, neither for general use.  This one was 
intended to check its argument and possibly return, exactly what its expansion 
did (after mentally tossing out the distracting "do while" noise).

Nobody competent to address logic errors in longobject.c was confused about 
that in the slightest.  Knowing the details of how small integers may be cached 
is part of what "competent" means in this context, and what the macro does is 
so simple it's no burden to learn or to use.

Is there an "aesthetic code cleanup" patch that's ever turned out well? ;-)

--
nosy: +tim.peters

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ma Lin]

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 this issue is: implicit return from macro.
We can add a comment before the call sites of CHECK_SMALL_INT macro, to explain 
that there is a possible return.

Revision: 6b519985d23bd0f0bd072b5d5d5f2c60a81a19f2
Author: animalize 
Date: 19-9-6 14:00:56
Message:
replace inline function `is_small_int` with a macro version (GH-15710)

Then this commit is not necessary.

Revision: c6734ee7c55add5fdc2c821729ed5f67e237a096
Author: Sergey Fedoseev 
Date: 19-9-12 22:41:14
Message:
bpo-37802: Slightly improve perfomance of PyLong_FromUnsigned*() (GH-15192)

This commit introduced a compiler warning due to this line [1]:
d:\dev\cpython\objects\longobject.c(412): warning C4244: “function”: from 
“unsigned long” to “sdigit ”，may lose data

[1] the line:
return get_small_int((ival)); \
https://github.com/python/cpython/blob/master/Objects/longobject.c#L386

Revision: 42acb7b8d29d078bc97b0cfd7c4911b2266b26b9
Author: HongWeipeng <961365...@qq.com>
Date: 19-9-18 23:10:15
Message:
bpo-35696: Simplify long_compare() (GH-16146)

IMO this commit reduces readability a bit.

We can sort out these problems.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

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 doesn't bode well and it makes it difficult 
to conduct reasoned discourse.  

(Meanwhile, we have another have another long-term active developer who 
routinely adds new macros to existing code because he thinks that is an 
improvement. That portends a pointless tug-of-war predicated almost solely in 
not liking how other people code).

--
assignee: rhettinger -> 

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

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

Victor, can you be more specific about the problem this is solving? I think the 
existing code in master really doesn't leave any problems in place that 
CHECK_SMALL_INT solves.

In master (as of 2702638ea), here's the code that CHECK_SMALL_INT would replace:

if (IS_SMALL_INT(ival)) {
return get_small_int((sdigit)ival);
}

When NSMALLPOSINTS=0 and NSMALLNEGINTS=0 , this expands in the preprocessor to 
the equivalent of:

if (0) {
return (Py_UNREACHABLE(), NULL);
}

(Specifically, it expands to whatever that expands to, since Py_UNREACHABLE and 
possibly NULL are also macros.)

A compiler that's attempting to do any significant optimization at all has to 
be able to discard code that's inside `if (0)`; dead-code elimination is a 
necessary primitive for lots of other optimizations.

(And at a quick empirical sanity-check: gcc, clang, and msvc all do so at any 
optimization setting other than "disabled".  In fact gcc and clang do so even 
with optimizations disabled.)


You made the case very nicely above that macros are evil.  The IS_SMALL_INT 
macro is fairly mild, and it has a performance benefit on some platforms to 
justify it.  But CHECK_SMALL_INT causes a return from the enclosing function, 
which seems quite high on the "evil macro" scale.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

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 replacing macros with functions when 
possible. I might be a bit biased, but it definitely makes the code a lot 
easier to understand (especially the macros with implicit returns). As long as 
there's not a significant performance loss and it doesn't introduce new 
complications, I don't see an issue with it.

>From my understanding, readability isn't as high of a priority in the C code, 
>but certainly there's some benefit to improving areas that are more difficult 
>to understand. The easier the code is to understand, the lower the overall 
>maintenance cost becomes.

Of course, this should certainly be done in moderation to reduce the 
introduction of unnecessary new bugs and the cost in reviewer time for more 
pressing concerns.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 15216, Benjamin and Raymond advised to replace the macro with a function.

I'm also a believer that one day, we could replace duplicated C functions 
accepting variant of C "integers" types with a single larger type:

* signed: replace (char, short, int, long, long long, ssize_t) with intmax_t
* unsigned: replace (unsigned char, unsigned short, unsigned int, unsigned 
long, unsigned long long, size_t) with uintmax_t

Sadly, it seems like such change has a small performance loss, and so cannot be 
done in "performance critical" code. I consider that longobject.c is such 
performance critical code.

--

I converted some macros of the Python C API to static inline functions, like 
Py_INCREF() or _PyObject_GC_TRACK():
https://vstinner.github.io/split-include-directory-python38.html

Compared to macros, static inline functions have multiple advantages:

* Parameter types and return type are well defined;
* They don't have issues specific to macros: see GCC Macro Pitfals 
https://gcc.gnu.org/onlinedocs/cpp/Macro-Pitfalls.html ;
* Variables have a well defined local scope ;
* GDB is able to retrieve then name when read the current C stack ("backtrace").

--

Again, I advice to add a small comment in longobject.c macros to explain that 
converting them to functions would loose performances.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

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 hoped, though I think we have different 
analyses of why that was.

(As you can probably guess, my previous message crossed with yours.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ma Lin]

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 to replace 
`Py_UNREACHABLE()` with `assert(0)`.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

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) using a macro *instead* of a plain old C function;
(b) using avoidable preprocessor conditionals.

And both of them led to that build failure in classic ways.

Specifically, the build failure happened because

(a) this `Py_UNREACHABLE` call was in an unusual syntactic context, squished 
into an expression, in order to use it in a macro.

(b) the call was behind an `#ifdef`/`#else`; and the configuration that 
included it wasn't one that got test-built by the authors of 3ab61473b (which 
modified `Py_UNREACHABLE`), nor by CI.

When conditional preprocessing is kept to a minimum -- here, if the `#if 
NSMALLNEGINTS + NSMALLPOSINTS > 0` conditional enclosed just the `small_ints` 
array that it needs to -- then this kind of build regression on non-default 
configurations can't so easily happen.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

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 generated code to be any different than it was).

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 so we can move 
on to something more substantive.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ma Lin]

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; r = range(256)" "consume(r)"  --duplicate=1000

I remember the results are:
inline function: 1.6  us
macro version  : 1.27 us
(32-bit release build by MSVC 2017)

Since the difference is too obvious, I tested it only once for each version.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

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 sequence, including a `return` that had been hidden inside the macro:

if (is_small_int(ival)) {
return get_small_int((sdigit)ival);
}

The second change converted `is_small_int` to a macro `IS_SMALL_INT`.

The second change also changed an `assert(0)` to say `Py_UNREACHABLE()`. I 
don't know why it did that -- it seems quite orthogonal to the rest of the 
change.


> Morever, if using a static inline function is causing issues,

The change that caused the build failure you're seeing (GH-15710) was intended 
for bpo-38015 .  Details there; briefly, common compilers on x86_32 would emit 
some unsightly extra instructions with `is_small_int`, and converting it to the 
macro `IS_SMALL_INT` eliminated those extra instructions.

It's not clear to me if anyone benchmarked to see if the conversion to a macro 
had any measurable performance benefit.  There's some measurement here:
https://bugs.python.org/issue38015#msg351315
which finds no benefit.  I'm not sure if that was measuring the change in 
GH-15710, or if it was an attempt to replicate the findings in msg351255 (which 
it quotes) about a more invasive change.

So I think the short of it is that the static inline function was not causing 
any issue, and an easy fix for the build failure is to revert GH-15710.

Alternatively, another easy fix would be to replace just the `Py_UNREACHABLE()` 
introduced by GH-15710 with the original `assert(0)`.

Either way, I don't think there's any reason here to revert GH-15216.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 a macro version (GH-15710)


This second change introduced a regression: bpo-38205 "Python no longer 
compiles without small integer singletons".

I don't understand the whole issue. A first change converted a macro to a 
static inline function. The second change converted the static inline fnuction 
to a macro... but the overall change introduced a regression. Why not reverting 
the first change instead of pushing a new change?

Morever, if using a static inline function is causing issues, it would be nice 
to add a comment to explain why, so the issue will be avoided in the future.

--
nosy: +vstinner
resolution: fixed -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

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 making code easier to read and less 
tricky. If the project continues to be successful for a long time to come, then 
that means the code will be read many, many more times than it's written. But 
one particular spot where it seems our experiences interestingly differ is:

> They are a bit tedious to review and are eating up our time in the back and 
> forth.

As a reviewer I generally find it much less work to review a change when it's 
intended to have no effect on the code's behavior. First, because it's easier 
to confirm no effect than to pin down what the effects are; then because the 
whole set of questions about whether the effects are desirable doesn't arise. 
As a result I often ask contributors (to Zulip, say) to split a change into a 
series of small pure refactors, followed by a very focused diff for the 
behavior change.

So that's certainly background to my sending as many PRs that don't change any 
behavior as PRs that do.

I actually have quite a number of draft changes built up over the last few 
weeks. I've held back on sending them all at once, partly because I've felt I 
have enough open PRs and I wanted to get a better sense of how reviews go. 
Perhaps I'll go pick out a couple more of them that are bugfixes, features, and 
docs to send next.

(You didn't mention docs just now, but given the care I see you take in adding 
to them and in revising What's New, I think we agree that work there is 
valuable.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

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 long term, but I definitely understand your point.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

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)
https://github.com/python/cpython/commit/5e63ab05f114987478a21612d918a1c0276fe9d2


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

Change by Raymond Hettinger :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

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 total code volume.  There are subtle differences during the transformation 
(macros don't require type declarations). They are a bit tedious to review and 
are eating up our time in the back and forth. All changes like this risk 
introducing bugs into otherwise correct code.  Minor code churn is rarely worth 
it.

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Change by Kyle Stanley :


--
type:  -> enhancement

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

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

-CHECK_BINOP(self, other);
+if (!PyLong_Check(self) || !PyLong_Check(other)) {
+Py_RETURN_NOTIMPLEMENTED;
+}

with the names `self` and `other` varying from site to site.  Though the 
expanded version isn't literally a `return` statement, I think it functions 
almost as well as one for the purposes that explicitness serves here, and much 
better so than a reference to `CHECK_BINOP`.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

Change by Greg Price :


--
pull_requests: +15140
pull_request: https://github.com/python/cpython/pull/15448

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

Raymond Hettinger  added the comment:

python-dev likely isn't the right place.  That is a forum for more mature ideas.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ma Lin]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

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 python-ideas maillist.  Small integer operations (including) indexing 
used to be smaller and faster in Python 2.7, so there is likely some room for 
improvement on what we have now.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ma Lin]

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 
including the cost of overflow checking.

If operation will overflow or other operation like **, we can transform native 
int to current form and run in current code path.

--
nosy: +Ma Lin

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

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 quite clean, though. Just sent, as GH-15216 -- 
please take a look.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

Change by Greg Price :


--
pull_requests: +14944
pull_request: https://github.com/python/cpython/pull/15216

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

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 
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.  See: 
https://en.wikipedia.org/wiki/There_are_known_knowns

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

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 like there must be something about their setup
that I'm not imagining, because this is a pretty tiny amount of memory (9
kB) on the scale of a typical Python server process in my experience.

Or do you mean they were increasing the numbers? One thought I had looking
at this was actually that it would be interesting to do some measurements
and try to pick new (default) values for these parameters - it looks like
they're the same today as in 1993, and it's likely that with all that's
changed in typical hardware since then (plus the switch from Python ints +
longs to only what we used to call longs), the optimum values today are
different from the optimum in 1993. And certainly one of the effects of
this optimization when its hit rate is good is reducing memory consumption
- you have only one 17 instead of however many you make.

I'll rework the explicit-return patch to send without this change. It'll be
a little trickier but should be entirely doable.

On Sat, Aug 10, 2019, 23:19 Raymond Hettinger 
wrote:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

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 in order to identify identity test bugs.  Also, 
eveonline was controlling this to save memory.

--
assignee:  -> rhettinger
nosy: +rhettinger

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

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 to send it as a 
PR after the first is merged? Happy to do another workflow if that'd make 
review easier. (I don't see guidance in the devguide on handling a sequence of 
patches, so I'm making this up.)

--
nosy: +aeros167, mark.dickinson, sir-sigurd

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

Change by Greg Price :


--
keywords: +patch
pull_requests: +14932
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/15203

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Greg Price]

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
   invokes undefined behaviour when ival is LONG_MIN */
abs_ival = 0U-(unsigned long)ival;
sign = -1;
}
else {
// ... etc. etc.

The CHECK_SMALL_INT macro contains a `return`, so the function can actually 
return before it reaches any of the other code.

#define CHECK_SMALL_INT(ival) \
do if (-NSMALLNEGINTS <= ival && ival < NSMALLPOSINTS) { \
return get_small_int((sdigit)ival); \
} while(0)

That's not even an error condition -- in fact it's the fast, hopefully 
reasonably-common, path.

An implicit return like this is pretty surprising for the reader. And it only 
takes one more line (plus a close-brace) to make it explicit:

if (IS_SMALL_INT(ival)) {
return get_small_int((sdigit)ival);
}

so that seems like a much better trade.

Patch written, will post shortly.

--
components: Interpreter Core
messages: 349359
nosy: Greg Price
priority: normal
severity: normal
status: open
title: Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)
versions: Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com