Page 1 of 2

Bug in polymake 2.10

Posted: 28 Oct 2011, 14:42
by Max
Hi there,
since I couldn't find any public bug tracker for polymake, nor any other information as to how to report bugs, except for a pointer to the forum in the "AQ" section, I am reporting this here now:

In polymake 2.10, file include/core/polymake/internal/sparse2d_ruler.h:151 says the following:

Code: Select all

if (diff<=0 ? -diff>m : (n_alloc=r->_alloc_size+std::max(diff, m)), true) {
I was compiling polymake using clang, which tends to give better diagnostics than GCC. It warned about the expression "-diff>m" being unused. Indeed, this code looks incorrect to me. I guess this was meant instead:

Code: Select all

if (diff<=0 ? -diff>m : ((n_alloc=r->_alloc_size+std::max(diff, m)), true)) {
Personally, I would avoid such needlessly "clever" code and just write

Code: Select all

if (diff<=0 ? -diff>m : true) { if (diff > 0) n_alloc=r->_alloc_size+std::max(diff, m);
which is clear and not as prone to hard-to-spot errors.

Re: Bug in polymake 2.10

Posted: 28 Oct 2011, 17:21
by Max
Here is another bug uncovered by clang: In include/core/polymake/internal/constant_containers.h:80:

Code: Select all

bool operator< (const single_value_iterator& it) const { return !at_end && it.at_end; }
This will *always* return false, as at_end is a pointer to a member method -- while _at_end is the value that is really needed in this text. So this is the correct code:

Code: Select all

bool operator< (const single_value_iterator& it) const { return !_at_end && it._at_end; }
The same mistake is repeated in line 88.

Re: Bug in polymake 2.10

Posted: 28 Oct 2011, 17:28
by Max
A third bug in include/core/polymake/Set.h:110

Code: Select all

return iterator(get_set().find_nearest(limit, polymake::operations::gt()), get_set.end());
should be

Code: Select all

return iterator(get_set().find_nearest(limit, polymake::operations::gt()), get_set().end());

Re: Bug in polymake 2.10

Posted: 29 Oct 2011, 11:21
by Max
Another compiler error one gets with clang is in lib/core/include/internal/type_manip.h:1827

Code: Select all

template <typename Result, typename Source, typename _Generic>
Here _Generic is a reserved identifier, used for some C1X extensions. A simple workaround is to change this to Generic_.

Note: According to the C++03 standard (and the ISO C99 standard it is based on), all identifiers beginning with an underscore followed by an upper-case letter or another underscore are always reserved, in all namespace. Some references for this: <http://c-faq.com/decl/namespace.html> and <http://stackoverflow.com/questions/2287 ... identifier>

Polymake makes liberal use of such identifiers (for things like _Set, _Iterator, _Operation, _Matrix, _Vector etc.), which is bound to cause issues with more C++ compilers sooner or later. E.g. GCC will likely implement the "generic selections" extensions one day, too.

Re: Bug in polymake 2.10

Posted: 30 Oct 2011, 23:34
by gawrilow
Thanks a lot for pointing to all these bugs! Indeed, they remained undiscovered by gcc, because they sat in a code written "for the stock", which was never instantiated so far. The fixes will appear in the next release. All identifiers starting with an underscore and a capital letter have gone as well.

Re: Bug in polymake 2.10

Posted: 31 Oct 2011, 13:20
by Max
Glad to hear you could fix all of this. :)

There are more changes that are necessary for to make clang happy, though, and more warnings that are not critical but look as if it would be nice to fix them. E.g. polymake mixes "class" and "struct" when referring to a type. Harmless but trivial to fix.

Another problem is that sparse_proxy(_it)_base::iterator is protected in both types, which GCC accepts but triggers an error in clang. Looking at the code, I believe clang is right to complain.

Then there are some constructs GCC accepts but clang rejects -- and clang is right according to the C++ standard, so it's likely future GCC versions will eventually reject the code, too. Example: the definition of the ListValueOutput& operator<< (const X& x) method uses class Value before it is defined; in GCC this may be accepted, perhaps because the method is only instantiated *after* class Value has been declared, but this is a bug in GCC.

There are various more errors and warnings, which I recorded in a local git repository I made out of polymake 2.10. This also includes a patch which makes configure.pl a bit more clever about detecting compilers. I.e. it does not anymore assume that every compiler is either Intel C++, or else an (possible outdated) GCC version... ;). But I am not sure whether these changes are still relevant / applicable for your secret development version of the code.

Re: Bug in polymake 2.10

Posted: 31 Oct 2011, 16:21
by Max
Oh, and lest I be misunderstood: I think polymake is doing exceedingly well when compiled with clang; all "issues" it found are very minor ones, which is very good for a project of this size which makes so heavy use of template techniques. It also uncovered at least one compiler bug in clang itself ;). So, please don't read this all as a complaint, it's just a factual list of minor issues you may want to correct.

Re: Bug in polymake 2.10

Posted: 01 Nov 2011, 00:02
by gawrilow
You are absolutely right regarding ListValueOutput::operator<<, its definition is indeed misplaced.
But I couldn't localize any references to the protected iterators of sparse_proxy outside of these classes. Could you please point me to them?

Regarding "secret" code repository: if you are going to contribute any useful fixes or new functionality, you are welcome! You do know very well, whom to ask for an access :-)

Re: Bug in polymake 2.10

Posted: 01 Nov 2011, 12:11
by Max
The "iterator" error seems to be triggered by e.g. the DeclTypedefCHECK(iterator) in core/polymake/internal/iterators.h:700.
Attached to this post is a .zip file with a text file iterator-clang-err.txt that contains one set of these warnings. Unfortunately I don't know how to copy the nice coloring of the output clang++ produces (copy&paste into TextEdit.app didn't work). But I hope the output is readable enough.

The .zip file also includes some of my less-hackish patches, maybe some of them will be useful to you. Since clang is not yet supported, you may want to modify the configure.pl patch to error out when clang is detected; but I think having clean detection of GCC and ICC will be helpful regardless of this.

Regarding the "secret" code repository: That was a tongue-in-cheek comment, sorry :). Calling it "internal" is better. The GAP CVS repository is currently "internal only", too. I am not happy with that, and hope it'll change when the project switches to Mercurial (and I gave a talk on the subject, trying to sway my fellow developers), but for now this is the same situation in any case.

Re: Bug in polymake 2.10

Posted: 01 Nov 2011, 16:38
by Max
There is another error which I can't figure out (present with Apple clang 2.0 and 3.0 and also the latest development version of clang). In particular, I am not sure whether this a problem in Polymake, or one in clang. Basically, it appears as if some code is to be compiled that is meant to multiply a (diagonal) matrix by an integer. But there is no operator* doing that, and so it errors out. And indeed, I can't find such an operator. But that just proves that I am insufficient and don't know the polymake codebase well enough -- in fact I am not even sure whether my interpretation of what is going on is correct :).

So it's best if somebody who actually knows polymake well could take a look and see if they can spot where the issues is. If it is definitely not a problem in polymake, then I'll try to narrow it down to a reproducible test case for the clang folks. For this, I'd appreciate if somebody could adjust my interpretation of what is going on accordingly (unless you want to file a bug report yourself, of course).

The error message is in the attached file.