[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Ok, i personally think this patch should go in. I think it makes it clear enough that an unsuspecting user would suspect something by reading the flag, and makes the feature hard enough to discover. And i'm happy to support anybody who's going to work on this stuff.

-----

~*sorry for hijacking review with this, we should move this conversation to the list*~

In https://reviews.llvm.org/D46159#1088768, @pfultz2 wrote:

> I am definitely interested in working on getting some of the checkers stable, and have already started some work on expanding the ConversionChecker, but would like user feedback as I move forward.


Yay! I would most likely have a chance to help out with final evaluation of checkers before they move out of alpha.

In https://reviews.llvm.org/D46159#1088768, @pfultz2 wrote:

> Some like the Conversion checker doesn't seem to have any issues(because its fairly limited at this point).


Last time i tried it, it was very hard to understand positives produced by the checker. It seems that it needs to implement a relatively sophisticated "bug visitor" in order to explain why does it think that a certain conversion would overflow by highlighting the respective spot somewhere in the middle of the path. It is especially important when the value comes from an inlined function because the analyzer wouldn't draw paths through all inlined functions (it would have been overwhelming) unless something interesting happens in there.

Then somebody would need to have a look at intentional overflows and if it's possible to avoid them.

In https://reviews.llvm.org/D46159#1088768, @pfultz2 wrote:

> Others like StreamChecker probably just needs some simple fixes.


Hmm, at a glance, it looks almost empty.

1. Again, it needs a bug visitor to highlight where the file was open. Otherwise many reports may become impossible to understand.
2. It is eagerly splitting states upon file open, which doubles remaining analysis time on every file open. This needs to be deferred until an actual branch is found in the code.
3. It needs a pointer escape callback, in order to work with wrappers around open/close functions. This needs to be complemented with a certain knowledge about the standard library, to see what functions should not cause escapes. SimpleStreamChecker has an example.
4. If it's ever to support `open()`/`close()`, it would, apart from other things, also need an "integer escape" callback , which will require a bit of digging into the analyzer core.

Pt.4 is mostly about feature work, but Pt.1–3 are must-fix. This isn't an overwhelming amount of work, but it will take some time.

Otherwise, yeah, it's a good simple check to have. It could be expanded to support a variety of weird APIs such as `fdopen()` but it's also a checker that's hard to get wrong. PthreadLockChecker is also a great candidate with similar problems; i have some unfinished work on that on here on the phabricator.


https://reviews.llvm.org/D46159