enable -Wconversion and -Wsign-conversion by default

[minad]

disable -Wsystem-headers

[minad]

I also disable the warnings in the mpi tests, but this is code we might not want to touch?

[minad]

@sjaeckel Note that the only changes are to the tests, no changes to the library code.

[minad]

Does not build cleanly on travis. It did on my machine, I have to check.

[minad]

Fortunately we have this nice travis setup which tests the different configuration 👍 Fix is on the way.

[minad]

@sjaeckel About these warnings - you have to say if we really want them activated in the case of MP_8BIT and MP_16BIT since in those cases the code gets quite a bit uglier. The issue is that mp_word is smaller than int and every calculation is promoted to int which would result in many casts and for example the += operator on mp_words could not be used anymore.

My recommendation would be to activate the warnings only for sizeof(mp_word) >= 4 cases. What do you think?

[minad]

My recommendation would be to activate the warnings only for sizeof(mp_word) >= 4 cases. What do you think?

I am trying this. Let's see.

[minad]

@sjaeckel It works like this. Squashed and rebased.

[sjaeckel]

why not keep them enabled by default?

[minad]

There are conversion warnings in the system headers unfortunately. I also don't think Wsystem-headers is particularly useful since we cannot do anything about those issues. I never activate these warnings since they are only intended for people working on the system headers it seems.

[minad]

But with NO_CONV_WARNINGS=1 for the 16 and 8 bit builds we also have the warnings activated at least sometimes in the CI, which is better than nothing :)

[minad]

@sjaeckel rebased!

[czurnieden]

@minad My Travis is not yet running, so you still might make it first! ;-)

[minad]

Hmm, for some reason the build is failing again :(

[minad]

I guess it is because of xenial. Did the compiler version change?

[minad]

Now it is gcc 5.4, before that it was 4.8.

[czurnieden]

Took a closer look. You can try with some more casting inside the bit-pushing but I fear a complete change to unsigned char for all variables and values involved would be necessary.
But as I'm a strong advocate of the more warnings are resolved earlier, the less problems later I would like to keep it if possible but not at all cost, of course.

Now it is gcc 5.4, before that it was 4.8

Should not matter. Only the other way around or if it is a known bug and this warning is correct, not a bug. But that 4.8 did not recognise it is good to know because it is my default GCC, too, so I need to do the checks with a younger one.

[minad]

Hmm, I am a bit inclined to give up. On my systems I am using much newer compilers (gcc 7 or 8) and very recent clangs, where these issues don't occur anymore. My interest in fighting old compilers is very limited.

I think this PR only makes sense if no heavy changes to the code are needed. For example I've read that the issue won't occur if you store the right side of |= first in an unsigned char temporary variable. But this is just stupid.

[sjaeckel]

Should we probably simply update gcc on Travis?

[minad]

My favorite solution would be to test against multiple gcc and clang versions. For example gcc 6, 7, 8 and clang 5, 6, 7. It is the question if you want to blow up the build matrix or just pick out the demo test and only build this one against multiple versions.

[sjaeckel]

To test the standard build with all those versions should suffice IMO

[minad]

Let's keep this PR open until #208 is resolved.

[czurnieden]

xenial (Ubuntu 16.04) with GCC 5.4 is a LTS and ends in 2021. We might get more restrictive but at least the LTS distributions of the Big Players(TM) have to be supported. Ubuntu 14.04 LTS runs out this month (which runs on this machine, argh!) so GCC 4.8 can fall out if necessary.
(I think I type too slow ;-) )

[minad]

closed via #225