[cfe-dev] PList output for clang Tidy

[Gábor Horváth]

Hi Alex,

I agree that this involves significant amount of code duplication. I also
agree that extending DiagnosticsEngine would be a cleaner solution (however
that would require massive amount of modification through the clang and
static analyzer codebase). If you think a patch with similar design may be
accepted I will start sending patches via Phabricator.

I see the following options:
- Clean up this patch and attempt to make it accepted to support plists and
extend the DiagnosticsEngine later.
- Clean up this patch and attempt to make it accepted to support plists and
do not bother with DiagnosticEngine for now.
- Drop this design and start to extend DiagnosticEngine. Add plist support
later.

Which option do you suggest?

Thanks,
Gábor

On 30 March 2015 at 18:00, Alexander Kornienko <alexfh at google.com> wrote:

> Hi Gåbor,
>
> The problem with this approach is that it results in a significant code
> duplication:
>   * the whole error filtering logic will need to be duplicated for
> the AnalyzerDiagnosticConsumer that handles plist output (note that
> diagnostic filtering won't work for plist output of analyzer results in
> your patch). And it seems that it will require making significantly more
> changes to ento::PlistDiagnostics than just making it public and inheriting
> from it.
>   * plist-formatting the output of non-analyzer diagnostics to a certain
> degree duplicates what is being done in PlistDiagnostics (and duplication
> will likely increase as the output formats for these two kinds of
> diagnostics are brought closer together).
>
> As I've noted earlier, extending DiagnosticsEngine would be one possible
> solution (probably not the easiest one, though). Maybe pulling out some
> reusable parts will allow to reduce code duplication while basically having
> the same design as you propose. I suspect however that the former approach
> can result in a more maintainable code.
>
> P.S. I would appreciate if you sent patches via Phabricator for a more
> convenient review. Thanks!
>
> -- Alex
>
> On Fri, Mar 27, 2015 at 12:50 PM, Gábor Horváth <xazax.hun at gmail.com>
> wrote:
>
>> Hi,
>>
>> I had some free time this week and I started to implement a prototype for
>> clang tidy plist output (patches are attached to this mail). In this first
>> attempt I tried to keep clang changes minimal. To achieve this, running
>> clang tidy generates two separate plist output: one for the reports that
>> are generated by the clang static analyzer and one for the reports
>> generated by tidy checkers. I try to make these two kinds of plists as
>> similar as possible. I did not add note and fixit information to the plists
>> yet and the plists which contains reports from tidy checkers do not contain
>> a file list. I choose this separate plist design because the
>> PathDiagnosticConsumers and DiagnosticConsumers have different life times.
>> Supporting one output would add too much extra complexity in my opinion
>> (unless the clang diagnostic engine learns to handle path diagnostics).
>>
>> What is your opinion about the design? Should I continue the
>> implementation or should I change the design?
>>
>> Regards,
>> Gábor
>>
>>
>>
>> On 12 January 2015 at 16:10, Alexander Kornienko <alexfh at google.com>
>> wrote:
>>
>>> Hi Gábor,
>>>
>>> On Mon, Jan 5, 2015 at 11:21 AM, Gábor Horváth <xazax.hun at gmail.com>
>>> wrote:
>>>
>>>> On 5 January 2015 at 11:02, Manuel Klimek <klimek at google.com> wrote:
>>>>
>>>>> +chandler and daniel; I think that are all the chefs we need for this
>>>>> particular porridge
>>>>>
>>>>> On Mon Jan 05 2015 at 10:05:02 AM Gábor Horváth <xazax.hun at gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Hello everyone,
>>>>>>
>>>>>> The Clang Static analyzer can output the diagnostics in plist
>>>>>> format. It is a useful feature, because it is easy to parse that format
>>>>>> with 3rd party tools, hence integrating clang tools with others.
>>>>>>
>>>>>> Unfortunately the plist reporting format is not supported by Clang
>>>>>> Tidy. We would like to add plist support to it. This involves a lot of
>>>>>> changes both to the format and the public API, so I want your opinion, how
>>>>>> to do it.
>>>>>>
>>>>>> In my opinion we need to extend the plist format to:
>>>>>> * Support notes that are not events
>>>>>> * Support fixits
>>>>>>
>>>>>> What do you think, what would be the bast way to extend the format
>>>>>> with those informations?
>>>>>>
>>>>>
>>> Ted or Anna can say whether it's possible to extend the plist Static
>>> Analyzer output format and what's the best way to do this. Also, as you
>>> noted, most of PlistDiagnostics should be factored out and exposed in a
>>> form of some API to be reusable. This can be done completely independent of
>>> the ClangTidy part of the changes. Ted, Anna and Jordan should be the right
>>> people to send relevant patches to.
>>>
>>>
>>>> The plist reporting related functionality is not part of the Clang
>>>>>> public API at the moment. The best would be, if the Static Analyzer and
>>>>>> regular diagnostics could be reported to the same plist output file. To
>>>>>> achieve this, the diagnostic consumer that outputs to the plist should
>>>>>> support both PathDiagnostics and regular Diagnostics. It would be
>>>>>> redundant to reimplement the whole functionality in Clang Tidy. To reduce
>>>>>> the redundancy, we would like to refactor several plist related helper
>>>>>> functions out from PlistDiagnostics and make it available in public
>>>>>> headers. We would also like to make PlistDiagnostics class available
>>>>>> so we can inherit from it. What do you think, what would be the best way to
>>>>>> organize these changes?
>>>>>>
>>>>>
>>>>> If I remember correctly, Chandler has long ago proposed to merge the
>>>>> different diagnostic types we have into one central clang diagnostic type
>>>>> that supports all our use cases.
>>>>>
>>>>
>>> Chandler, do you have any thoughts on this?
>>>
>>>
>>>> I personally would need to see a CL to judge whether it makes sense,
>>>>> but generally, I think what you say sounds like it's the right direction
>>>>> (if you agree with the sentence above: make clang's basic diagnostic system
>>>>> powerful enough to support the analyzer's use cases, and then switch the
>>>>> analyzer and clang-tidy to use it).
>>>>>
>>>>>
>>>> In my proposal it would be possible to create a Diag Consumer that can
>>>> consume both PathDiagnostics and regular Diagnostics. It would be possible,
>>>> to make the same object to consume both Clang and Static Analyzer diags.
>>>>
>>>
>>> The problem is not consuming messages from the Static Analyzer in a
>>> different format (ClangTidy already subscribes to PathDiagnostics), but the
>>> fact that ClangTidy uses Clang DiagnosticsEngine internally to handle
>>> diagnostics from the Static Analyzer, from all checks, and from the
>>> compiler. It means that to handle all information from PathDiagnostic we'd
>>> need to either extend Clang's diagnostic subsystem or rewrite a significant
>>> part of ClangTidy to introduce an alternative way to handle PathDiagnostics
>>> inside ClangTidy. It seems to me that the former is a much superior
>>> solution.
>>>
>>> In any case we'd also need to extend the ClangTidyError structure (used
>>> to return ClangTidy results to clients) to accommodate additional
>>> information from PathDiagnostic.
>>>
>>> It may not be as clean as merging the two kinds of diagnostics, but I
>>>> suspect this approach is faster to implement. In the long term I do agree
>>>> that, it would be desirable to use the same mechanisms for both Static
>>>> Analyzer and Clang. But I am not sure that, I have enough time for a
>>>> refactoring like that.
>>>>
>>>>
>>>>>
>>>>> I'd vote for renaming PList to something non-horrible in the process,
>>>>> (I still have no idea what the "P" stands for), but that's bikeshedding.
>>>>>
>>>>
>>>> I don't know if we could rename that. PList is not Clang specific,
>>>> Apple uses this format in other projects as well.
>>>>
>>>
>>>>
>>>>>
>>>>> Cheers,
>>>>> /Manuel
>>>>>
>>>>>
>>>>
>>>> Thanks,
>>>> Gábor
>>>>
>>>
>>>
>>> Regards,
>>> Alex
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150330/ec7b9974/attachment.html>