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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

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

> I think the premise is a bit off the mark. It's not that these are not for the common user -- it's that they're simply not ready for users at all. Making it easier to expose does not seem like it serves users because those users expect exposed features to work.


That was also the sentiment static analyzer folks were voicing at some point. I also sympathize to the idea of testing checks and contributing fixes to them, but what the CSA maintainers seem to dislike is a stream of bugs for alpha checkers from users expecting of a certain level of support. So it's basically their decision whether they want to expose alpha checkers via clang frontend and/or via clang-tidy. I can only say whether I like the specific way it is done in clang-tidy.

> 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.

"We've fixed the glitch" by making everyone wanting a bugzilla account send an email to a human. So only the users who pass this sort of a Turing test will file bugs ;)



================
Comment at: clang-tidy/ClangTidyOptions.h:80-82
+  /// \brief Turns on experimental alpha checkers from the static analyzer.
+  llvm::Optional<bool> AllowEnablingAlphaChecks;
+
----------------
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.


================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:198
+/// because it is highly not recommended for users.
+static cl::opt<bool> AllowEnablingAlphaChecks("allow-enabling-alpha-checks",
+                                              cl::init(false), cl::Hidden,
----------------
s/checks/checkers/ (in the static analyzer's terminology)

I would also make it more explicit that these are static analyzer checkers. -allow-enabling-clang-static-analyzer-alpha-checkers or something like that.

The variable name could be AllowEnablingAnalyzerAlphaCheckers, for example.


================
Comment at: test/clang-tidy/enable-alpha-checks.cpp:2
+// Check if '-allow-enabling-alpha-checks' is visible for users
+// RUN: clang-tidy -help | not grep 'enable-alpha-checks'
+
----------------
grep 'allow-enabling-alpha-checks'


https://reviews.llvm.org/D46159