[issue15530] Enhance Py_MIN and Py_MAX

[Martin v . Löwis]

Martin v. Löwis added the comment:

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

--

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[Meador Inge]

Meador Inge added the comment:

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.

--

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[Martin v . Löwis]

Changes by Martin v. Löwis :


--
resolution:  -> rejected
status: open -> closed

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[Martin v . Löwis]

Martin v. Löwis added the comment:

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

--

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[Meador Inge]

Changes by Meador Inge :


--
nosy: +meador.inge

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[Antoine Pitrou]

Antoine Pitrou added the comment:

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

--

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[Martin v . Löwis]

Martin v. Löwis added the comment:

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

--

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[Antoine Pitrou]

Antoine Pitrou added the comment:

Le vendredi 03 août 2012 à 12:15 +, 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.

--

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[Martin v . Löwis]

Martin v. Löwis added the comment:

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.

--
Added file: http://bugs.python.org/file26674/min.c

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[Antoine Pitrou]

Antoine Pitrou added the comment:

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

--

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[Antoine Pitrou]

Antoine Pitrou added the comment:

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

--
nosy: +pitrou

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[STINNER Victor]

STINNER Victor added the comment:

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

--
versions: +Python 3.4 -Python 3.3

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[Martin v . Löwis]

Martin v. Löwis added the comment:

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?

--
nosy: +loewis

___
Python tracker 

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

[issue15530] Enhance Py_MIN and Py_MAX

[STINNER Victor]

New submission from STINNER Victor:

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

--
components: Interpreter Core
files: py_min_max.patch
keywords: patch
messages: 167161
nosy: haypo
priority: normal
severity: normal
status: open
title: Enhance Py_MIN and Py_MAX
versions: Python 3.3
Added file: http://bugs.python.org/file26651/py_min_max.patch

___
Python tracker 

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