This revision was automatically updated to reflect the committed changes.
Closed by commit rL332609: [clang-tidy] Add a flag to enable alpha checkers
(authored by alexfh, committed by ).
Herald added subscribers: llvm-commits, klimek.
Changed prior to commit:
pfultz2 updated this revision to Diff 146719.
pfultz2 added a comment.
Rebased
https://reviews.llvm.org/D46159
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/tool/ClangTidyMain.cpp
alexfh added a comment.
In https://reviews.llvm.org/D46159#1098018, @alexfh wrote:
> In https://reviews.llvm.org/D46159#1098000, @pfultz2 wrote:
>
> > Is someone able to merge in my changes?
>
>
> Will do.
Please rebase the patch, it doesn't apply cleanly.
https://reviews.llvm.org/D46159
alexfh added a comment.
In https://reviews.llvm.org/D46159#1098000, @pfultz2 wrote:
> Is someone able to merge in my changes?
Will do.
https://reviews.llvm.org/D46159
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
pfultz2 added a comment.
Is someone able to merge in my changes?
https://reviews.llvm.org/D46159
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
pfultz2 updated this revision to Diff 145925.
pfultz2 added a comment.
Some changes based on feedback.
https://reviews.llvm.org/D46159
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
aaron.ballman accepted this revision.
aaron.ballman added a comment.
Aside from a minor commenting nit, also LGTM.
Comment at: clang-tidy/tool/ClangTidyMain.cpp:195
+/// This option allows enabling alpha checkers from the static analyzer, that
+/// are experimental. This
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
Given Artem's answer (and if there are no objections from other CSA
maintainers), I have no concerns with this patch going in. A couple of minor
nits.
Comment at:
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.
-
pfultz2 added a comment.
> In this sense bug reports against abandoned alpha checkers (which are,
> unfortunately, the majority) aren't very useful. In most cases it's just too
> easy to see how broken they are.
Although the majority are like that, not all of them are. Some like the
NoQ added a comment.
In https://reviews.llvm.org/D46159#1088050, @alexfh wrote:
> 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
pfultz2 updated this revision to Diff 145229.
pfultz2 added a comment.
Moved flag for alpha checks to the ClangTidyContext
https://reviews.llvm.org/D46159
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
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
pfultz2 added a comment.
There doesn't seem to be a simple way to remove it from the ClangTidyOptions
class, as a lot more functions need to be changed to support that. I would
prefer to leave it there for now, and we can refactor it later. Either way, the
check can't be set from a config
pfultz2 updated this revision to Diff 145072.
pfultz2 added a comment.
Rename flag to `AllowEnablingAnalyzerAlphaCheckers`.
https://reviews.llvm.org/D46159
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidyOptions.cpp
clang-tidy/ClangTidyOptions.h
clang-tidy/tool/ClangTidyMain.cpp
pfultz2 added inline comments.
Comment at: clang-tidy/ClangTidyOptions.h:80-82
+ /// \brief Turns on experimental alpha checkers from the static analyzer.
+ llvm::Optional AllowEnablingAlphaChecks;
+
alexfh wrote:
> Since this will only be configurable via a
aaron.ballman added a comment.
In https://reviews.llvm.org/D46159#1086627, @alexfh wrote:
> 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
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
xbolva00 added inline comments.
Comment at: clang-tidy/tool/ClangTidyMain.cpp:195
+/// This option Enables alpha checkers from the static analyzer, that are
+/// experimental. This option is set to false and not visible in help, because
lebedev.ri wrote:
>
lebedev.ri added inline comments.
Comment at: clang-tidy/tool/ClangTidyMain.cpp:195
+/// This option Enables alpha checkers from the static analyzer, that are
+/// experimental. This option is set to false and not visible in help, because
xbolva00 wrote:
>
pfultz2 added a comment.
> When something is merged into Clang trunk, the expectation is that it will be
> production quality or will be worked on rapidly to get it to production
> quality, which is somewhat orthogonal to getting user feedback. I don't know
> that I have the full history of
aaron.ballman added a comment.
In https://reviews.llvm.org/D46159#1086553, @pfultz2 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.
>
> Then why was it merged into clang in the
pfultz2 updated this revision to Diff 145029.
pfultz2 added a comment.
Rename flag to `allow-enabling-alpha-checks` and removed the option from the
clang-tidy file.
https://reviews.llvm.org/D46159
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidyOptions.cpp
xbolva00 added a comment.
In https://reviews.llvm.org/D46159#1086553, @pfultz2 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.
>
> Then why was it merged into clang in the first
pfultz2 added a comment.
> 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.
Then why was it merged into clang in the first place? It seems like the whole
point of merging it into clang is to get
xbolva00 added inline comments.
Comment at: clang-tidy/tool/ClangTidyMain.cpp:195
+/// This option Enables alpha checkers from the static analyzer, that are
+/// experimental. This option is set to false and not visible in help, because
This option enables...
aaron.ballman added a comment.
In https://reviews.llvm.org/D46159#1086472, @lebedev.ri wrote:
> In https://reviews.llvm.org/D46159#1086463, @pfultz2 wrote:
>
> > > As Devin says (and as we discussed this with Anna Zaks) alpha checkers
> > > are still in development, so we don't want to expose
alexfh added a comment.
In https://reviews.llvm.org/D46159#1086487, @alexfh wrote:
> In https://reviews.llvm.org/D46159#1086479, @pfultz2 wrote:
>
> > > But still, could you explain the use case and why a local modification of
> > > clang-tidy is not an option?
> >
> > Because I would like to
alexfh added a comment.
In https://reviews.llvm.org/D46159#1086479, @pfultz2 wrote:
> > But still, could you explain the use case and why a local modification of
> > clang-tidy is not an option?
>
> Because I would like to direct users to try an alpha check on their local
> codebases without
pfultz2 added a comment.
> But still, could you explain the use case and why a local modification of
> clang-tidy is not an option?
Because I would like to direct users to try an alpha check on thier local
codebases without having to tell them to rebuild clang.
Repository:
rCTE Clang Tools
lebedev.ri added a comment.
In https://reviews.llvm.org/D46159#1086463, @pfultz2 wrote:
> > As Devin says (and as we discussed this with Anna Zaks) alpha checkers are
> > still in development, so we don't want to expose them to the users, even
> > very curious ones.
>
> Then why do we make
alexfh added a comment.
In https://reviews.llvm.org/D46159#1086332, @lebedev.ri wrote:
> In https://reviews.llvm.org/D46159#1086322, @alexfh wrote:
>
> >
>
>
> How about `-enable-alpha-checks=yes-i-know-they-are-broken` ?
A hidden flag with a scary name without any way to specify it in
pfultz2 added inline comments.
Comment at: clang-tidy/ClangTidy.cpp:373-376
// FIXME: Remove this option once clang's cfg-temporary-dtors option defaults
// to true.
AnalyzerOptions->Config["cfg-temporary-dtors"] =
Context.getOptions().AnalyzeTemporaryDtors ?
alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.
Comment at: clang-tidy/ClangTidy.cpp:373-376
// FIXME: Remove this option once clang's cfg-temporary-dtors option defaults
// to true.
pfultz2 added a comment.
> As Devin says (and as we discussed this with Anna Zaks) alpha checkers are
> still in development, so we don't want to expose them to the users, even very
> curious ones.
Then why do we make them available with `clang --analyze`? If the plan is not
to expose them to
lebedev.ri added a comment.
In https://reviews.llvm.org/D46159#1086322, @alexfh wrote:
>
How about `-enable-alpha-checks=yes-i-know-they-are-broken` ?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46159
___
cfe-commits mailing
alexfh added a comment.
As Devin says (and as we discussed this with Anna Zaks) alpha checkers are
still in development, so we don't want to expose them to the users, even very
curious ones. For those who want to help with development of the alpha
checkers, there's always a possibility to
lebedev.ri added a comment.
In https://reviews.llvm.org/D46159#1085684, @dcoughlin wrote:
> In https://reviews.llvm.org/D46159#1085548, @pfultz2 wrote:
>
> > > Do you have some better choices?
> >
> > I could do `-allow-alpha-checks`. What do you think?
>
>
> That seems reasonable to me.
>
dcoughlin added a comment.
In https://reviews.llvm.org/D46159#1085548, @pfultz2 wrote:
> > Do you have some better choices?
>
> I could do `-allow-alpha-checks`. What do you think?
That seems reasonable to me. Although I like `-allow-enabling-alpha-checks`
better because it is longer and will
pfultz2 added a comment.
> Do you have some better choices?
I could do `-allow-alpha-checks`. What do you think?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46159
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
lebedev.ri added a comment.
In https://reviews.llvm.org/D46159#1085400, @pfultz2 wrote:
> Do you want the flag to be renamed?
I personally would //like// a bit better name, but i'm not sure what would be
better, so not a requirement.
Do you have some better choices?
Repository:
rCTE
pfultz2 added a comment.
Do you want the flag to be renamed?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46159
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
pfultz2 added a comment.
> Am i understanding correctly that the new -enable-alpha-checks flag doesn't
> enable alpha checks, but it only allows enabling them with a separate command?
Yes, it enables them to be selected by clang tidy.
Repository:
rCTE Clang Tools Extra
pfultz2 added a comment.
> I'm curious about the use case for this, since I don't think it ever makes
> sense to run all the alpha checks at once. These checks are typically a work
> in progress (they're not finished yet) and often have false positives that
> haven't been fixed yet.
We want
lebedev.ri added a comment.
In https://reviews.llvm.org/D46159#1081559, @NoQ wrote:
> Am i understanding correctly that the new `-enable-alpha-checks` flag doesn't
> enable alpha checks, but it only allows enabling them with a separate command?
Yes, absolutely! Same with my followup
NoQ added a comment.
Am i understanding correctly that the new `-enable-alpha-checks` flag doesn't
enable alpha checks, but it only allows enabling them with a separate command?
Also wanted to mention that developers who are interested in running analyzer
alpha checkers over a compilation
NoQ added inline comments.
Comment at: clang-tidy/ClangTidy.cpp:373-376
// FIXME: Remove this option once clang's cfg-temporary-dtors option defaults
// to true.
AnalyzerOptions->Config["cfg-temporary-dtors"] =
Context.getOptions().AnalyzeTemporaryDtors ? "true"
lebedev.ri added a comment.
In https://reviews.llvm.org/D46159#1081392, @dcoughlin wrote:
> In https://reviews.llvm.org/D46159#1081371, @lebedev.ri wrote:
>
> > Note that it is completely off by default, and is not listed in
> > documentation, `clang-tidy --help`,
> > so one would have to know
dcoughlin added a comment.
In https://reviews.llvm.org/D46159#1081371, @lebedev.ri wrote:
> Note that it is completely off by default, and is not listed in
> documentation, `clang-tidy --help`,
> so one would have to know of the existence of that hidden flag first, and
> only then when that
lebedev.ri added a comment.
In https://reviews.llvm.org/D46159#1081366, @dcoughlin wrote:
> ...
Note that it is completely off by default, and is not listed in documentation,
`clang-tidy --help`,
so one would have to know of the existence of that hidden flag first, and only
then when that
dcoughlin added a comment.
I'm curious about the use case for this, since I don't think it ever makes
sense to run all the alpha checks at once. These checks are typically a work in
progress (they're not finished yet) and often have false positives that haven't
been fixed yet. Often they don't
lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.
So i suspect the history here is: https://reviews.llvm.org/D26310 added a way
to filter-out these clang analyzer checks,
which removed these checks from being available in clang-tidy,
lebedev.ri added a comment.
In https://reviews.llvm.org/D46159#1080939, @lebedev.ri wrote:
> Oh, hm.
> I just needed something to view the call graph.
> I have almost wrote a clang-tidy check using `analysis/CallGraph.h`, but
> then i noticed/remembered that clang static analyzer has that
lebedev.ri added a comment.
Oh, hm.
I just needed something to view the call graph.
I have almost wrote a clang-tidy check using `analysis/CallGraph.h`, but then i
noticed/remembered that clang static analyzer has that already.
But `$ clang-tidy -checks=* -list-checks | grep -i analyzer | grep
lebedev.ri added a comment.
I just feel like pointing out that you can already do that:
$ clang-tidy -checks=clang-analyzer-alpha* -p <>
(be wary of `*` expanding)
`clang-tidy --help` says:
-checks= -
<...> This option's value is appended
pfultz2 created this revision.
pfultz2 added reviewers: aaron.ballman, hokein, ilya-biryukov.
Herald added subscribers: cfe-commits, xazax.hun.
The alpha checkers can already be enabled using the clang driver, this allows
them to be enabled using the clang-tidy as well. This can make it easier
56 matches
Mail list logo
