<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 4 Sep 2019 at 01:17, Artem Dergachev <<a href="mailto:noqnoqneo@gmail.com">noqnoqneo@gmail.com</a>> wrote:<br></div><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 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></div><div><br></div><div>In any case, your changes to BugReport gave much needed clarity to the code! :)</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">
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">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>