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.

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

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.

P.S.  Making the returns explicit is a reasonable change to make, but the core #ifdef logic should remain.

> 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 <report@bugs.python.org>
wrote:

>
> Raymond Hettinger <raymond.hettinger@gmail.com> added the comment:
>
> P.S.  Making the returns explicit is a reasonable change to make, but the
> core #ifdef logic should remain.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue37812>
> _______________________________________
>

> 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

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

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.

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.

I sent twice, but it doesn't appear in Python-Ideas list.
I will try to post to Python-Dev tomorrow.

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

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

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

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

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

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

Sadly, GH-15710 was merged without mentioning the bpo-37812 (this issue) in the final commit message :-(

commit 6b519985d23bd0f0bd072b5d5d5f2c60a81a19f2
Author: animalize <animalize@users.noreply.github.com>
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.

Victor, I agree that both changes should be reverted.  We should avoid churning long stable, bug free code for minimal aesthetic value.

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.

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

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.

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

> 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)`.

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

Greg, would you be willing to work on a PR that reverts pr15216 and pr15710 while preserving the efforts of pr15192 ?

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.

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.

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

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.

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

Recent commits for longobject.c

    Revision: 5e63ab05f114987478a21612d918a1c0276fe9d2
    Author: Greg Price <gnprice@gmail.com>
    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 <animalize@users.noreply.github.com>
    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 <fedoseev.sergey@gmail.com>
    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 <961365124@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.

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

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

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

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