Issues according to coverity (tc_xor fixed, fgets not yet fixed)

[minad]

I think this is a false positive but by restructuring the conditions we could avoid it. Still it would make sense to set up travis ci coverity.

[minad]

I registered https://scan.coverity.com/projects/libtom-libtommath but it is not yet activated there.

[czurnieden]

This is a false positive indeed, the software cannot know that sign only holds two values—that it is a pseudo-bool, so to speak. All static analysers I know have some special kind of comment to handle this situations (vid. e.g.: splint). I doubt they don't have those but Google couldn't find a list for me.

Coverty (their online version) does not allow for a lot of scans per week, only about two dozens or so for our small (~12,000 loc says cloc(1)) project. Would that be a problem?

What about other static analysers? There are a lot that are open-source which (not all at once, of course ;-) ) we could pack into a makefile_test_static_analyser, or something alike?

And what's about dynamic testing? If I handle memory manually (malloc et al.) I always do a test-run with Valgrind (mostly just valgrind --leak-check=full --show-leak-kinds=all).

And like above: pack into a makefile_test_dynamic_analyser, or something alike?

[minad]

Yes, the issue was introduced by @fperrad's linting. Before that, I had written it in a more sloppy style, which just checked for truthiness. This is also why it would make sense to go to C99 ;)

Concerning other static analysers - I am regularily running clang analyze (scan-build). There are not many other useful ones imho.

Valgrind is also very useful and I am using that very often. Furthermore it makes sense to use sanitizers which do a bit more or less than valgrind depending on situation but at better performance.
However I had more issues with sanitizers, which could not handle some things.

[czurnieden]

This is also why it would make sense to go to C99 ;)

Nope, sorry, c90 it is. To much legacy ballast. (doesn't Debian still insist on c90, too?)

However I had more issues with sanitizers, which could not handle some things.

So does Valgrind. Makes quite a mess with thelong double. Uses double instead but won't tell anybody (e.g.: does not reset the limits in float.h), although you can include valgrind.h to check if you run inside Valgrind. Took the liberty to add a simple switch in demo.c, see README.md. It's a bit how-ya-doing but didn't want to add another dependency.

[minad]

I don't understand where you've heard that debian requires c90. There are packages in debian which are not c90 and not even c ;) Why should they care?

Valgrind cannot handle long double? I didn't know since I've never used long double. But in general valgrind should have any problems since it emulates the cpu. The only issue I had was with very large mmaps (~ TB), which slow it down. The memory mappings were not used physically, it was just allocated virtual address space. For that case I also added a check if valgrind is running, like you suggested.

What you can do - check for __has_include, check then for __has_include(<valgrind.h>) and then you don't add a depedency this way.

[czurnieden]

I don't understand where you've heard that debian requires c90

It once popped up in one of my diagnostic tests for the Debian build. Can't remember where and how only that it popped up. Hence the parentheses and the question mark.

check for __has_include

See #160

[minad]

The second issue found by coverity:

[czurnieden]

Forgot it the first time, but here it is now: no pictures, please, thank you!

Yes, that is one: must check fgetc for EOF.
And initialize ch with e.g.: '0' to sooth "coverty".

[minad]

@czurnieden What is the problem with pics and what do you suggest as quick alternative? I am pragmatic. Even emacs can show pics ;)

[czurnieden]

@minad

Even emacs can show pics

But not everyone can view them.
Also: pics are not searchable; neither by Google nor by grep
So: don't use pics if avoidable (not at all costs, of course).

[minad]

I agree in general :)

[czurnieden]

And before someone asks: "But whattabout the out-of-bounds error?!?!", there is none. The cast to unsigned folds over in case the result of ch - (int)'(' is negative and the result of the folding makes (mp_s_rmap_reverse_sz < pos) always true if sizeof(int) is large enough.

The check of fgets is not that necessary but it would get mp_fread a more deterministic behaviour.

Also: the I/O functions do not get tested very thoroughly in demo.c now. Should they?

[minad]

@czurnieden Hmm, maybe it would make sense to create a new well structured test suite (additional to what is there now). Like one test_mp_* function per mp_* function or something like this. Then it would also be more obvious what is being tested and what is not.

[czurnieden]

That would be a lot of work!
And you are willing to do it?
Respect!