Enable LGTM for code analysis (Semmle) (and Facebook Infer)

[XVilka]

Using these tools will help to eliminate obvious errors. For zstd it is important from the security point of view, and C/C++ are supported. See:

• https://lgtm.com/
• https://github.com/marketplace/lgtm

Another tool (from your company, haha) with similar capabilities is Infer, it works with both C and C++, helping to find some bugs that are not overlapping with Coverity or LGTM (Semmle) due to the theory used. It's open source at GitHub too, but please be sure to use the version from master - there are some important bugs for C/C++ code were fixed recently and din't made into the release yet. There are docker images as well, to simplify the installation.

[Cyan4973]

Hi @XVilka

Thanks for the suggestion. We already use a static analyzer as part of our Continuous Integration suite.

While it can be useful to look at additional static analyzers once in a while (and we do), it's not a good idea to multiply the nb of analyzers that are run continuously at each commit and PR. The reasons for this are :

• Static analyzers are full of false positive. The signal to noise ratio is actually pretty low.
• A good CI policy is "zero-warning tolerance". That means that all issues must be solved, including false positives. This quickly becomes a tedious workload.
• Multiple static analyzers will feature multiple kind of false positives, sometimes applying to the same code but in different ways leading to :
  • torteous code, trying to please multiple constraints, hurting readability and therefore maintenance. Sometimes, such complexity introduce other bugs, that are out of scope of the analyzers.
  • sometimes, these constraints are mutually exclusive : if one try to solve one, the other static analyzer will complain, they can't be both happy at the same time.
• As if that was not enough, the list of false positives change with each version. So it's hard enough to follow one static analyzer, but multiple ones with their own update agenda, nope, that's crazy.

This is different from running a static analyzer once in a while, looking at the output, and cherry picking the warnings that seem to help, either because they detected a genuine risk of bug (rare) or because it helps expressing the code in a way which is more readable or safer (harder to break).

These kind of reports can be useful, and we do accept them from time to time.

[luben]

@Cyan4973 , totally agree with you. But I think @XVilka didn't express them right. The LGTM static analysis work asynchronously so it does not interfere with the workflow. You can consult the reports when you decide. BTW, it's already running on the Zstd releases code base in my repo and you can look at the report: https://lgtm.com/projects/g/luben/zstd-jni/alerts

I personally have not found any bug through it but have fixed some obvious code smells.

[Cyan4973]

That's a good point @luben , we could certainly have a look at https://lgtm.com/projects/g/facebook/zstd/?mode=list from time to time, since it's already listed there, and cherry pick a few warnings worth exploring.