A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Thanks for the reviews!

I have made the requested changes; please review again.

Thanks for making the requested changes!

@rhettinger: please review the changes made to this pull request.

Using inlined function generates unnecessary machine code than using macro.
I built a 32-bit Release version with VC2017, below is the starting code of PyLong_FromLong function.

The ival was first converted to a long long type, this is unnecessary .

use inlined function:

_PyLong_FromLong PROC					; COMDAT

; 317  : {

	push	ebp
	mov	ebp, esp
	and	esp, -8					; fffffff8H
	push	ecx
	mov	ecx, DWORD PTR _ival$[ebp]

; 318  :     PyLongObject *v;
; 319  :     unsigned long abs_ival;
; 320  :     unsigned long t;  /* unsigned so >> doesn't propagate sign bit */
; 321  :     int ndigits = 0;
; 322  :     int sign;
; 323  : 
; 324  :     if (is_small_int(ival)) {

	mov	eax, ecx
	push	ebx
	push	esi
	cdq
	push	edi
	xor	edi, edi

; 48   :     return -NSMALLNEGINTS <= ival && ival < NSMALLPOSINTS;

	add	eax, 5
	adc	edx, edi
	test	edx, edx
	ja	SHORT $LN6@PyLong_Fro
	jb	SHORT $LN32@PyLong_Fro
	cmp	eax, 261				; 00000105H
	ja	SHORT $LN6@PyLong_Fro
$LN32@PyLong_Fro:

; 60   :     v = (PyObject *)&small_ints[ival + NSMALLNEGINTS];

	movsx	eax, cx
	shl	eax, 4
	add	eax, OFFSET _small_ints+80
; File d:\dev\cpython\include\object.h

; 459  :     op->ob_refcnt++;

	inc	DWORD PTR [eax]
; File d:\dev\cpython\objects\longobject.c

; 383  : }

	pop	edi
	pop	esi
	pop	ebx
	mov	esp, ebp
	pop	ebp
	ret	0

use macro:

_PyLong_FromLong PROC					; COMDAT

; 318  : {

	push	ebp
	mov	ebp, esp
	mov	eax, DWORD PTR _ival$[ebp]
	push	edi

; 319  :     PyLongObject *v;
; 320  :     unsigned long abs_ival;
; 321  :     unsigned long t;  /* unsigned so >> doesn't propagate sign bit */
; 322  :     int ndigits = 0;

	xor	edi, edi

; 323  :     int sign;
; 324  : 
; 325  :     if (IS_SMALL_INT(ival)) {

	cmp	eax, -5					; fffffffbH
	jl	SHORT $LN6@PyLong_Fro
	cmp	eax, 257				; 00000101H
	jge	SHORT $LN6@PyLong_Fro

; 60   :     v = (PyObject *)&small_ints[ival + NSMALLNEGINTS];

	cwde
	shl	eax, 4
	add	eax, OFFSET _small_ints+80
	pop	edi
; File d:\dev\cpython\include\object.h

; 459  :     op->ob_refcnt++;

	inc	DWORD PTR [eax]
; File d:\dev\cpython\objects\longobject.c

; 384  : }

	pop	ebp
	ret	0

I built a 32-bit Release version with VC2017, below is the starting code of PyLong_FromLong function.

Interesting, thanks!

It looks like several compilers produce code much like this when compiling for x86-32. Here's a Compiler Explorer example I just created:
https://godbolt.org/z/8qPNez
The pane on the left has source code that models the relevant bits of ours. The lower right has two different compilers compiling for x86-32.

On the other hand, as you can see in the same Compiler Explorer example (in the upper-right pane), VC2017 compiling for x86-64 produces essentially identical code for the macro version and the static-inline-function version. So does Clang; and in fact, because in my example both are present, GCC notices they're identical and makes the macro version just jump immediately to the function version 😄

I believe the longer code represents a missed optimization by the compilers; I don't think there's any semantic reason why (on any architecture) they have to emit anything different for the function version vs. the macro version.

In any case, stepping back: we're talking about a handful of additional instructions, of the kinds a CPU is very fast at (no random reads from memory, which can take thousands of cycles and which the interpreter by its nature is full of); also only in 32-bit builds, not 64-bit. It's interesting to see the code that's generated and study it... but convincing the compiler to generate one of these versions over another is very much a micro-optimization, the kind that generally isn't worth more than a very small delta in the simplicity of the code.

For more fun, I just went and added to the Compiler Explorer example a version with IS_SMALL_INT as a macro:
https://godbolt.org/z/8GVhVR

With IS_SMALL_INT, we get the micro-optimization: all of MSVC, GCC, and Clang optimize the code just as well as they do the CHECK_SMALL_INT version, for x86-32 as well as for x86-64. The IS_SMALL_INT version isn't much more complex than is_small_int; the main gnarliness was in CHECK_SMALL_INT.

So, switching from is_small_int to IS_SMALL_INT (as in the original version of this PR) would arguably be an OK trade. Though I still wouldn't be enthusiastic about doing so; in general I'd rather use regular C-level functions, with all their goodies like proper type-checking and visibility to debuggers and other tools, everywhere possible, and use a preprocessor macro only in cases where truly only a macro can do the job. (In this file, the two WRITE_DIGITS macros are good examples of that; there are many others in the codebase.)

If build a 32-bit version with gcc 9.2, it also generates unnecessary machine code (convert to long long) as above:
https://godbolt.org/z/qna2eG

Same with ARM gcc 8.2: https://godbolt.org/z/qycDzR

long long appears in the argument of inlined function, maybe we can't ignore it.

VC2017 compiling for x86-64 produces essentially identical code for the macro version and the static-inline-function version

In this case, the long -> long long converting is still there, but doesn't use too much machine code:
https://godbolt.org/z/-78POZ

GCC notices they're identical and makes the macro version just jump immediately to the function version

I know why, on 64-bit Windows, long is 4-byte. But on 64-bit GCC, long is 8-byte, so long and long long are the same thing.
https://stackoverflow.com/questions/384502/what-is-the-bit-size-of-long-on-64-bit-windows

long long appears in the argument of inlined function, maybe we can't ignore it.

No, compilers have a lot more freedom than that. The code just needs to compute the right answer. The types in the source are part of defining what the right answer is, but there doesn't need to be anything at runtime that physically looks like them (in a case like this where they aren't part of any external interface.)

Here's a quick demo of that:
https://godbolt.org/z/1Bj9KG
It's a tiny simplified model of the is_small_int logic, compiled for x86-32 by both GCC and Clang. In this version, GCC still emits the extra few instructions to extend a 32-bit int to a 64-bit long long... but Clang sees that the behavior doesn't actually depend on the extra bits, and it optimizes down to very tight code that skips that and works only on 32-bit values. In fact the code it produces is exactly the same if you edit the source to replace long long with int.

No, compilers have a lot more freedom than that. The code just needs to compute the right answer. The types in the source are part of defining what the right answer is, but there doesn't need to be anything at runtime that physically looks like them (in a case like this where they aren't part of any external interface.)

If one day int128_t type and PyLong_FromInt128 function are used in the code, converting to long long generates wrong result.
So using a certain type long long is not a good idea.

I have no opinion on the use of inlined function or macro here.
There are no wrong results now, just a little performance loss.

If one day int128_t type and PyLong_FromInt128 function are used in the code, converting to long long generates wrong result.
So using a certain type long long is not a good idea.

That's true (provided long long in the same environment was smaller than an int128_t). For that reason, it'd be better to write intmax_t instead of long long. I'd forgotten about intmax_t when I wrote this PR, but a related PR of Sergey's reminded me of it: #15192 (comment)

It seems that all the major compilers have generated unnecessary code.
It's an unnecessary waste of performance, although very tiny. I created issue38015 for this.