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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D46159#1086732, @aaron.ballman wrote:

> If the static analyzer people desire this feature, that would sway my position on it, but it sounds like they're hesitant as well. 
>  However, I don't think clang-tidy is beholden either -- if we don't think this functionality should be exposed and can justify that position, that should carry weight as well. \
>  From a policy perspective, I would be fine with a policy for clang-tidy where we never expose an alpha checker from the static analyzer (or only expose checks on a case by case basis) because I don't mind users having to jump through hoops to get to experimental, unsupported functionality.


As folks noted here, some users prefer to use clang-tidy as a frontend for the static analyzer. If this helps them test experimental CSA features and CSA maintainers are willing to accept bug reports and potentially patches from this category of users, I don't want to create obstacles, as long as all experimental features need to be explicitly enabled.

Devin, what's your take on this?

> I primarily don't like the fact that, as a user, I enable checks by name but for some kinds of checks I have to *also* enable them via a secondary mechanism otherwise the name doesn't even exist. This strikes me as being a likely source of confusion where forgetting one flag causes behavioral differences the user doesn't expect.

This particular aspect doesn't seem problematic to me. In order to observe these behavioral differences, the user would have to specify this flag at least once, get the results of experimental checkers and see the disclaimer, if we decide to add one (see below). I guess, forgetting about the existence of this flag would be quite unlikely. Forgetting the spelling of the flag should not cause any confusion - it can be looked up in the source code, found out using `clang-tidy -help-hidden` or asked about.

>>> Making the flag sound scary doesn't suffice -- many users never see the flags because they're hidden away in a build script, but they definitely see the diagnostics and file bug reports.

As an additional way to ensure that this flag doesn't get specified by mistake / doesn't get buried and forgotten inside a script, clang-tidy can print a disclaimer each time it's executed with this flag.



================
Comment at: clang-tidy/ClangTidyOptions.h:80-82
+  /// \brief Turns on experimental alpha checkers from the static analyzer.
+  llvm::Optional<bool> AllowEnablingAlphaChecks;
+
----------------
pfultz2 wrote:
> alexfh wrote:
> > Since this will only be configurable via a flag, this option will be global (i.e. not depend on the location of the file being analyzed). I'd suggest to remove this option altogether and use something else to pass it to ClangTidyASTConsumerFactory. It could be stashed into ClangTidyASTConsumerFactory and passed as a parameter of runClangTidy,  or it could live in ClangTidyContext.
> But it needs to be passed to `getCheckNames` as well, which doesn't take a `ClangTidyContext`.
We can add a boolean parameter to `getCheckNames`. `ClangTidyASTConsumerFactory` could get this as a constructor parameter or from `ClangTidyContext`.


https://reviews.llvm.org/D46159