Page MenuHomePhabricator

[clang-tidy] Add -enable-alpha-checks command
AbandonedPublic

Authored by Prazek on Jan 14 2017, 5:32 AM.

Details

Event Timeline

Prazek updated this revision to Diff 84447.Jan 14 2017, 5:32 AM
Prazek retitled this revision from to [clang-tidy] Add -enable-alpha-checks command.
Prazek updated this object.
Prazek added a reviewer: alexfh.
Prazek added a subscriber: cfe-commits.
Prazek updated this revision to Diff 84448.Jan 14 2017, 5:36 AM

reformat

aaron.ballman accepted this revision.Jan 14 2017, 6:06 AM
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a subscriber: aaron.ballman.

LGTM with a few minor nits, but in the future, please provide some summary of what your patch is going to do rather than leave it entirely blank. It makes it easier on the reviewers if they have an idea of what to expect and why it's needed. ;-)

Also, this should probably be added to the release notes.

clang-tidy/ClangTidyOptions.h
78

from static-analyzer -> from the static analyzer

This revision is now accepted and ready to land.Jan 14 2017, 6:06 AM
alexfh requested changes to this revision.Jan 14 2017, 2:04 PM
alexfh edited edge metadata.

As discussed with the Static Analyzer maintainers, alpha checkers are completely unsupported and are suitable for very early testing only. We had problems with them routinely, that's why I disabled alpha checkers in clang-tidy completely. I don't think there should be a user-visible way to enable them. Developers can locally change the code to get access to alpha checkers, but released binaries shouldn't provide this possibility.

This revision now requires changes to proceed.Jan 14 2017, 2:04 PM
alexfh added inline comments.Jan 14 2017, 2:11 PM
clang-tidy/ClangTidy.cpp
296

This is the place where a small local change will enable alpha checkers, if needed. It specifically shouldn't be configurable by users. It might make sense to add a comment to this effect here.

As discussed with the Static Analyzer maintainers, alpha checkers are completely unsupported and are suitable for very early testing only. We had problems with them routinely, that's why I disabled alpha checkers in clang-tidy completely. I don't think there should be a user-visible way to enable them. Developers can locally change the code to get access to alpha checkers, but released binaries shouldn't provide this possibility.

That's good to know -- should it be documented a bit more explicitly, or perhaps the alpha checks should be removed until they're fit for public consumption? Some of those alpha checks have been in the product for a long time, and if they're so unstable that we cannot expose them in a user-friendly fashion, perhaps they don't belong yet?

As discussed with the Static Analyzer maintainers, alpha checkers are completely unsupported and are suitable for very early testing only. We had problems with them routinely, that's why I disabled alpha checkers in clang-tidy completely. I don't think there should be a user-visible way to enable them. Developers can locally change the code to get access to alpha checkers, but released binaries shouldn't provide this possibility.

That's good to know -- should it be documented a bit more explicitly,

Yes, a comment to that effect near the relevant code wouldn't hurt. Or do you have other suggestions?

or perhaps the alpha checks should be removed until they're fit for public consumption? Some of those alpha checks have been in the product for a long time, and if they're so unstable that we cannot expose them in a user-friendly fashion, perhaps they don't belong yet?

As discussed with SA folks, alpha checkers are convenient for them to develop new checks, but shouldn't be exposed to users. Some of these experimental checkers might deserve being moved out of alpha or removed completely, but that should be reported to and discussed with the SA maintainers.

So the problem I got is that every time I want to check if there is already a feature in clang-tidy/static-analyzer that solves my issue, I either have to deal with static-analyzer command line, which is horrible, or I have to modify and recompile the source code.

The case I was looking at is a check to find

int *p = g():
*p = 42;
if(!p) {g(): }

I wanted to find null checks after the pointer was dereferenced, and indeed I found it in deadcode or unreachable code analysis in static-analyzer.

I am not sure what you are afraid of - as long as this feature is not turned by default everything should be fine. If someone would turn it on then as long as he doesn't see any false positives then everything is still fine, and it can always disable flag.

AFAIK there is a way to specify in cl::opt that the option is hidden, which could probably solve your concerns?

aaron.ballman requested changes to this revision.Jan 15 2017, 7:53 AM
aaron.ballman edited edge metadata.

As discussed with the Static Analyzer maintainers, alpha checkers are completely unsupported and are suitable for very early testing only. We had problems with them routinely, that's why I disabled alpha checkers in clang-tidy completely. I don't think there should be a user-visible way to enable them. Developers can locally change the code to get access to alpha checkers, but released binaries shouldn't provide this possibility.

That's good to know -- should it be documented a bit more explicitly,

Yes, a comment to that effect near the relevant code wouldn't hurt. Or do you have other suggestions?

A comment in the code would be okay, but I think that removing all public mention of the alpha checkers (help text and website) would also be useful; I would not have thought that a production compiler would carry these checks for this many years if they weren't stable and useful.

or perhaps the alpha checks should be removed until they're fit for public consumption? Some of those alpha checks have been in the product for a long time, and if they're so unstable that we cannot expose them in a user-friendly fashion, perhaps they don't belong yet?

As discussed with SA folks, alpha checkers are convenient for them to develop new checks, but shouldn't be exposed to users. Some of these experimental checkers might deserve being moved out of alpha or removed completely, but that should be reported to and discussed with the SA maintainers.

Agreed.

As discussed with the Static Analyzer maintainers, alpha checkers are completely unsupported and are suitable for very early testing only. We had problems with them routinely, that's why I disabled alpha checkers in clang-tidy completely. I don't think there should be a user-visible way to enable them. Developers can locally change the code to get access to alpha checkers, but released binaries shouldn't provide this possibility.

That's good to know -- should it be documented a bit more explicitly,

Yes, a comment to that effect near the relevant code wouldn't hurt. Or do you have other suggestions?

A comment in the code would be okay, but I think that removing all public mention of the alpha checkers (help text and website) would also be useful; I would not have thought that a production compiler would carry these checks for this many years if they weren't stable and useful.

I don't quite understand. Are you suggesting:

  • removing help text
  • adding cl::hidden

What website are you talking about?

or perhaps the alpha checks should be removed until they're fit for public consumption? Some of those alpha checks have been in the product for a long time, and if they're so unstable that we cannot expose them in a user-friendly fashion, perhaps they don't belong yet?

As discussed with SA folks, alpha checkers are convenient for them to develop new checks, but shouldn't be exposed to users. Some of these experimental checkers might deserve being moved out of alpha or removed completely, but that should be reported to and discussed with the SA maintainers.

Agreed.

I don't agree that alpha checkers are that bad. When I was testing it year ago there were only few false positives, probably in 2 checkers.

As discussed with the Static Analyzer maintainers, alpha checkers are completely unsupported and are suitable for very early testing only. We had problems with them routinely, that's why I disabled alpha checkers in clang-tidy completely. I don't think there should be a user-visible way to enable them. Developers can locally change the code to get access to alpha checkers, but released binaries shouldn't provide this possibility.

That's good to know -- should it be documented a bit more explicitly,

Yes, a comment to that effect near the relevant code wouldn't hurt. Or do you have other suggestions?

A comment in the code would be okay, but I think that removing all public mention of the alpha checkers (help text and website) would also be useful; I would not have thought that a production compiler would carry these checks for this many years if they weren't stable and useful.

I don't quite understand. Are you suggesting:

  • removing help text
  • adding cl::hidden

I'm suggesting removing help text.

What website are you talking about?

Things like this: http://clang-analyzer.llvm.org/alpha_checks.html

To me, there's a difference between "prevented from being on by default" and "completely unsupported". If they're completely unsupported, we probably shouldn't expose them more easily, but we may want to clarify this in user-facing documentation and help text.

or perhaps the alpha checks should be removed until they're fit for public consumption? Some of those alpha checks have been in the product for a long time, and if they're so unstable that we cannot expose them in a user-friendly fashion, perhaps they don't belong yet?

As discussed with SA folks, alpha checkers are convenient for them to develop new checks, but shouldn't be exposed to users. Some of these experimental checkers might deserve being moved out of alpha or removed completely, but that should be reported to and discussed with the SA maintainers.

Agreed.

I don't agree that alpha checkers are that bad. When I was testing it year ago there were only few false positives, probably in 2 checkers.

If the maintainers of the static analyzer think they shouldn't be exposed to users, I'm inclined to follow that advice.

Prazek updated this revision to Diff 84550.Jan 16 2017, 5:24 AM
Prazek edited edge metadata.
Prazek marked an inline comment as done.

-enable-alpha-checks is now not visible for users, but it make
my life as clang-tidy developer much easier.

Does solution like this works for you? We don't officially support alpha checkers, but it is much easier to check if something is already implemented in static analyzer easily

Does solution like this works for you? We don't officially support alpha checkers, but it is much easier to check if something is already implemented in static analyzer easily

Is it the only problem you're trying to solve by this patch? If so, it might be better to add an alias/script to run clang --analyze -Xclang -analyzer-checker=alpha or even use gcc.godbolt.org (e.g. https://godbolt.org/g/msQigz).

My concern is that if there's a user-visible option (undocumented, unlisted, named --no-no-no-please-dont-use-this-option or otherwise discouraged), people will use it (sometimes unintentionally, because, well, they changed their config once to give it a try and then forgot about it for a year), rely on it, complain about the tool, etc. If clang-tidy is used in some sort of a server-side manner (e.g. integrated with code review), misbehaving checkers may cause crashes, and this is problematic, when maintainers have no intention to fix the bugs and well, they explicitly asked not to expose experimental checkers (see http://llvm.org/PR26855, for example).

alexfh requested changes to this revision.Jan 17 2017, 4:28 AM
This revision now requires changes to proceed.Jan 17 2017, 4:28 AM

Ok, I didn't know that it is this easy to run alpha checks from clang.

Prazek abandoned this revision.Jan 20 2017, 7:08 AM