Hello, On Wed, Apr 06, 2011 at 09:33:50AM +0200, Richard B. Kreckel wrote:
Alexei, does it really break the ABI? Whether we return an int here or a char won't change library symbol names and since chars are returned as ints, it won't change anything binary-wise.
Changing the return type might break the ABI, and I think it's better to be safe than sorry.
There are cases where clifford_max_label returns -1, so this is indeed broken.
clifford_max_label returns some magic number (-1 aka 255) to indicate that the expression does not contain any gamma matrices. It's perfectly fine to do so as long as no valid representation label coincides with that magic number. Let's forget about computer algebra for a second and consider the following (C) code: #include <stdlib.h> /* size_t */ int max_val_s(const unsigned int* data, size_t sz) { size_t i = 0; unsigned int val = 0; if ((!data) || (!sz)) return -1; /* -1 is just a magic number which means "the input is empty */ for (; i < sz; ++i) { if (val < data[i]) val = data[i]; } return val; } The max_val_s is perfectly fine as long as the input data does not contain values greater than INT_MAX (usually 2^31 - 1). unsigned int max_val_u(const unsigned int* data, size_t sz) { size_t i = 0; unsigned int val = 0; if ((!data) || (!sz)) return -1; /* -1 is just a magic number which means "the input is empty */ for (; i < sz; ++i) { if (val < data[i]) val = data[i]; } return val; } The max_val_u is fine, too, as long as the input values are less than UINT_MAX. The above statements still hold if one replaces int with char (INT_MAX with CHAR_MAX, and UINT_MAX with UCHAR_MAX, respectively). Thus, clifford_max_label is OK as long as representation labels are within [0, 127] range (no matter if char is signed or not).
So, I don't see a problem with Martin's patch. Did I miss anything?
Apart from a possible ABI change the patch is harmless. On the other hand, it's useless (see the explanation above), so I don't see any reason to apply it (as in "if it ain't broke, don't fix it"). Best regards, Alexei