Attached patch enhances Py_MIN and Py_MAX to check that types of both arguments are compatible at compile time. Checks are only done if the compiler is GCC.

The patch uses also Py_MIN and Py_MAX in more places. (The commit may be done in two parts.)

I think that's too late for 3.3. It's not a bug fix.

If we use this kind of feature, we either need to declare a minimum supported GCC version (any GCC older than 10 years can be dropped IMO), or check for the specific version of GCC in which all the necessary features were present.

Did you detect any errors in the Python code using this change? Wouldn't the compiler refuse to perform the comparison if the types were not compatible?

> I think that's too late for 3.3. It's not a bug fix.

Oops, I chose 3.3 instead of 3.4. Fixed.

> If we use this kind of feature, we either need to declare a minimum supported GCC version

typeof() and __builtin_types_compatible_p() were introduced to gcc in version 3.0 and 3.1.1. Do we plan to support GCC older than 3.1? :-)

> (any GCC older than 10 years can be dropped IMO)

GCC 3.1 was released 10 years ago. It's cheap to add a test on the GCC version to fallback on the simple "x<y?x:y" macro, but I don't know exactly which GCC versions are supported and I don't want to install a very old GCC version just to check this.

> Did you detect any errors in the Python code using this change?

Nope. But it avoids at least to evaluate an expression twice (if an argument is not a variable or a number, but a complex expression... like most new PyUnicode_*() macros). Even if I prefer to avoid expressions in macro arguments...

> Wouldn't the compiler refuse to perform the comparison
> if the types were not compatible?

I don't know exactly how __builtin_types_compatible_p() compare types. At least, I can say that int and unsigned int are considered as incompatible.

--

The Linux kernel uses "(void) (&_min1 == &_min2);" to check that types are compatible. I suppose that this expression is written for GCC. GCC only emits a warning, I chose Py_BUILD_ASSERT_EXPR() to emit an error instead.

I don't understand the point of your patch. Can you explain?

> I don't understand the point of your patch. Can you explain?

Yes. It's explained in the comment of the two macros:

"When compiled with GCC, check also that types of x and y are compatible at compile time."

So it adds a cheap santity check at compile time.

> Yes. It's explained in the comment of the two macros:
> 
> "When compiled with GCC, check also that types of x and y are
> compatible at compile time."

I'm sorry, that doesn't explain anything. The C compiler already checks
types for you. So what does it bring? And if it brings anything, why
should it be restricted to Py_MIN and Py_MAX?

Victor hinted that it would detect errors when combining int and unsigned int. To elaborate, see the attached min.c. It gives

[traditional MIN definition]
  [int, pointer]
min.c:18: warning: comparison between pointer and integer
min.c:18: warning: pointer/integer type mismatch in conditional expression
[static assert]
  [int, unsigned int]
min.c:20: error: size of array ‘type name’ is negative
  [int, double]
min.c:21: error: size of array ‘type name’ is negative
  [int, pointer]
min.c:22: error: size of array ‘type name’ is negative
min.c:22: warning: comparison between pointer and integer
min.c:22: warning: pointer/integer type mismatch in conditional expression

So compared to the traditional type checks:
a) this gives a hard compile error, whereas the existing check would only produce warnings
b) the existing min happily combines (int,unsigned) giving unsigned and (int, double) giving double; the new code will will reject such code.

I think the feature is somewhat desirable; I agree code combining different types in MIN or MAX is flawed - if it is intentional, asking for an explicit cast is not asking too much.

The only downside of the patch is that it uses a language extension. We should strive to reduce usage of language extensions, not increase it.

I also have a personal dislike of fanciness in code. Code should be clean, not cute.

Le vendredi 03 août 2012 à 12:15 +0000, Martin v. Löwis a écrit :
> So compared to the traditional type checks:
> a) this gives a hard compile error, whereas the existing check would
> only produce warnings

Warnings are quite visible already, and we try to silence them.

> I think the feature is somewhat desirable; I agree code combining
> different types in MIN or MAX is flawed - if it is intentional, asking
> for an explicit cast is not asking too much.

I don't agree. Trying to battle with C's semantics doesn't seem very
productive, especially if it's only done in a single pair of macros.
If we think that implicit type conversions are too laxist, we should
probably use a different set of compiler options.

>> I think the feature is somewhat desirable; I agree code combining
>> different types in MIN or MAX is flawed - if it is intentional, asking
>> for an explicit cast is not asking too much.
>
> I don't agree. Trying to battle with C's semantics doesn't seem very
> productive, especially if it's only done in a single pair of macros.

What do you disagree with? That "combining different types in MIN and MAX
is flawed"? Or that "asking for an explicit cast is not asking too much"?

Whether or not the patch is an appropriate measure is only the second
question - what I said is that the kind of code that it detects is indeed
flawed. If you disagree, can you kindly give an example where mixing types
in min and max would be legitimate?

For the specific case of mixing signed and unsigned, there is wide-spread
agreement that people should avoid it, and some compilers detect the flawed
code quite well. Some cases are defined to have undefined behavior; other
cases do have well-defined behavior, but many C developers are unaware of
what the exact semantics is.

Mixing integers with pointers is already detected by compilers sufficiently.

Mixing integers with floating point isn't really an issue in Python's
source code, so I don't worry about this.

> >> I think the feature is somewhat desirable; I agree code combining
> >> different types in MIN or MAX is flawed - if it is intentional, asking
> >> for an explicit cast is not asking too much.
> >
> > I don't agree. Trying to battle with C's semantics doesn't seem very
> > productive, especially if it's only done in a single pair of macros.
> 
> What do you disagree with? That "combining different types in MIN and MAX
> is flawed"? Or that "asking for an explicit cast is not asking too much"?

The former. If C allows it then what's the point of special-casing
Py_MIN and Py_MAX to disallow it? It will only catch a very small
fraction of cases anyway.

Again, if this is a serious issue (which I don't think it is), it would
be better handled by choosing the appropriate compiler options.

> The former. If C allows it then what's the point of special-casing
> Py_MIN and Py_MAX to disallow it?

"C allows it" includes cases like "C allows an the result to be
implementation-defined, or an implementation-defined signal to be
raised", and indeed, some compilers do raise signals in the cases
where they are allowed to.

I'd rather not have code in the Python implementation that raises
implementation-defined signals on some systems and gives
implementation-defined results on other systems.

> Again, if this is a serious issue (which I don't think it is)

I agree.

> it would be better handled by choosing the appropriate compiler options.

It might well be that this *is* the appropriate compiler option (even
though it's not a compiler command line flag, but a compiler language
extension).

In any case, there seems to be agrement that this is not a serious
issue (Victor said he didn't catch any new errors when using this),
so I'm rejecting the patch.

I am indifferent with respect to the use of the GCC extensions, but getting rid of the umpteen different implementations of MIN/MAX is a nice , albeit very minor, cleanup.

> I am indifferent with respect to the use of the GCC extensions, but  
> getting rid of the umpteen different implementations of MIN/MAX is a  
> nice , albeit very minor, cleanup.

I think that's a different issue from the one we have here, though
(which specifically targets using GCC extensions). I also agree that
combining the MIN/MAX implementation (naturally into Py_MIN/Py_MAX)
is desirable - I don't think that needs an issue.

I'm opposed to reducing the number of times of expression evaluation
on GCC (i.e. statement expressions). If there are cases where the
double evaluation has side effects, I'd rather have it fail on GCC
as well (and not just on MSVC).