<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<div class="moz-cite-prefix">On 9/3/19 5:31 PM, Kristóf Umann wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAGcXOD7UbUze9dToSRSpgRHL6jbLzhkZ6FPC1=t+g3TDw7vKtg@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">On Wed, 4 Sep 2019 at 01:17, Artem Dergachev <<a
href="mailto:noqnoqneo@gmail.com" moz-do-not-send="true">noqnoqneo@gmail.com</a>>
wrote:<br>
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">Ugh, i just realized
this.<br>
<br>
Clang-Tidy doesn't really need BugReporter; all it needs is
<br>
PathDiagnosticConsumer. (the word "Path" is rudimentary)<br>
</blockquote>
<div><br>
</div>
<div>Maybe its time to rename it! :)</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
It should be perfectly comfortable to build PathDiagnostics
by hand - it <br>
maps nicely to how you already build your warnings and
notes. There's no <br>
need to force you to do the job of packing your notes into
an auxiliary <br>
BugReport object and then use BugReporter to unwrap it back
into a <br>
PathDiagnostic. It makes a huge lot of sense for
path-sensitive reports <br>
to de-duplicate the work of turning a report into a
PathDiagnostic, but <br>
it doesn't really mean much for path-insensitive reports.</blockquote>
<div> </div>
<div>A bit of an unrelated question, does clang-tidy house
CFG-based checks, or only AST based ones? For our upcoming
reaching definitions analysis, we may even want a
CFGBugReport class.<br>
</div>
</div>
</div>
</blockquote>
<br>
For must-problems i suspect that the really good way to present them
to the user would be to present all reports simultaneously (eg., as
part of a single HTML report file). For example: "this variable is
never changed and may be declared as const (1) because this
if-branch in which it's assigned is in fact dead (2) because its
condition always evaluates to true (3)". These may be three bug
reports emitted by three different CFG-based checkers, however this
whole logical chain of "(1) because (2) because (3)" may be
impossible to comprehend unless we show all three reports together.
That could be fun to write a BugReporter for.<br>
<br>
<blockquote type="cite"
cite="mid:CAGcXOD7UbUze9dToSRSpgRHL6jbLzhkZ6FPC1=t+g3TDw7vKtg@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">Change of plans, i guess.</blockquote>
<div><br>
</div>
<div>In any case, your changes to BugReport gave much needed
clarity to the code! :)</div>
</div>
</div>
</blockquote>
<br>
Sure, i still plan to land it.<br>
<br>
<blockquote type="cite"
cite="mid:CAGcXOD7UbUze9dToSRSpgRHL6jbLzhkZ6FPC1=t+g3TDw7vKtg@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
On 8/14/19 12:23 AM, Dmitri Gribenko wrote:<br>
> Hi Artem,<br>
><br>
> On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <<a
href="mailto:noqnoqneo@gmail.com" target="_blank"
moz-do-not-send="true">noqnoqneo@gmail.com</a>> wrote:<br>
>> As i've been vaguely hinting on EuroLLVM, i plan to
invest some time in<br>
>> decreasing the gap between Clang-Tidy users and
Static Analyzer users<br>
>> and make sure it's always possible for our users to
take the best of<br>
>> both worlds. In particular, i'd like to make
Clang-Tidy easily<br>
>> integratable into all UIs that already integrate
the Static Analyzer.<br>
> I like the idea of integrating the two tools more
closely. From the<br>
> user's point of view, I think, ClangTidy and Clang
Static Analyzer are<br>
> doing more or less the same thing, but somehow have
completely<br>
> different workflows and integrations.<br>
><br>
>> Ideally i'd love to do the same to Clang-Tidy that
Clang-Tidy does to<br>
>> us: make the Static Analyzer load it as a library
and expose Clang-Tidy<br>
>> checks as its own, maybe in a separate package.
This is harder to do<br>
>> though, because Clang-Tidy lives in a separate repo
and also it's a hard<br>
>> sell to build it into the Clang binary. I'm open to
suggestions here: we<br>
>> can keep such integration under an off-by-default
CMake flag (which<br>
>> requires enabling compilation of clang-tools-extra)
or we may even use<br>
>> it as a run-time plugin (i mean, clang plugins are
super wonky, but this<br>
>> time it's actually fairly easy to avoid version
conflicts, as they all<br>
>> get compiled from the same sources simultaneously).<br>
> I would like to suggest something different: move Clang
Static<br>
> Analyzer to clang-tools-extra. Build it either as a
separate binary or<br>
> compile it into the clang-tidy binary. Then let `clang
-analyze`<br>
> delegate to that binary for backwards compatibility.<br>
><br>
> Clang Static Analyzer is being developed in the clang
repository for<br>
> historical reasons -- we didn't have clang-tools-extra
at the time the<br>
> project was started. The "non-core" nature of Clang
Static Analyzer is<br>
> also validated by the fact that it is an optional part
of Clang -- we<br>
> have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.<br>
><br>
> Long-term, my preference would be to have all static
analysis tooling<br>
> (Clang Static Analyzer, ClangTidy, any prospective
dataflow-based<br>
> analysis) to have a consistent UI -- including
invocation, integration<br>
> with build systems, configuration strategy etc. While
the<br>
> implementation of these tools is very different, I
think the audience<br>
> is largely the same.<br>
><br>
>> But regardless of how far do i end up going with
such integration, first<br>
>> thing first: i'll anyway need to teach Clang-Tidy
how to route its<br>
>> diagnostics through our diagnostic engine. This is
also the only thing<br>
>> that's absolutely necessary; the rest can always be
hacked up on the UI<br>
>> side.<br>
>><br>
>> It's a great question why didn't we extend Clang's
DiagnosticEngine to<br>
>> begin with, but instead made our own facility. I
don't know the answer<br>
>> to that; this happened way before i even learned
what an AST is :) We<br>
>> generally needed a much more sophisticated
diagnostic engine because our<br>
>> reports are much more complicated. While we could
try to unify these<br>
>> diagnostic engines completely, i don't think it's
worth it.<br>
>><br>
>> So i think it's more feasible to make Clang-Tidy's
diag() method return<br>
>> a proxy object that mimics the DiagnosticBuilder
interface but may also<br>
>> pump the diagnostics to an instance of the Static
Analyzer's<br>
>> BugReporter. I hope this would avoid API breakages
for clang-tidy checks.<br>
> I'm very concerned about introducing lookalike APIs
that don't behave<br>
> exactly the same way as original ones. In LLVM, we are
generally not<br>
> concerned with backwards compatibility for C++ APIs.
Instead, we<br>
> should be looking to keep our code maintainable in the
long run.<br>
><br>
> With that in mind, my suggestion is to refactor
ClangTidy to use<br>
> BugReporter instead of DiagnosticBuilder.<br>
><br>
> Dmitri<br>
><br>
<br>
</blockquote>
</div>
</div>
</blockquote>
<br>
</body>
</html>