[cfe-commits] [PATCH] Tests for formatter diagnostics + custom DiagnosticConsumer in API.

[Manuel Klimek]

On Mon, Jan 14, 2013 at 1:31 PM, Alexander Kornienko <alexfh at google.com>wrote:

> On Mon, Jan 14, 2013 at 12:48 PM, Manuel Klimek <klimek at google.com> wrote:
>
>> Can't we put those tests into clang-tools-extra? After all, the
>> diagnostic approach seems to be bound much more to the tool than the
>> library...
>>
>
> Yes, I can, if you think this is the most reasonable way to do it. But the
> only diagnostic we have to date is implemented in the library, and is
> tool-independent. So I'd better put tests closer to the library, rather
> than the tool.
>

Let me explain my reasoning a little more:
To me, clang-format is a tool that tries to format as much as possible. If
we find an error in a place where we cannot format, our errors are going to
be useless compared to errors a real compiler would output. Thus, I don't
really care that much about our errors. To me they're more a help in
debugging, and will basically completely lose their value once we don't
fail parsing valid code any more.
I also think that over time we'll take out more and more errors, as we're
finding better heuristics on how to format broken code.

So, in conclusion, I don't find our diagnostics important, so if we have a
tests that verifies that diagnostics get output at all (which would be a
test that needs to live with with the tool, because otherwise we can always
regress the tool), and tests for all the cases we want to actually format
(thus knowing they don't have errors), I think that I honestly don't care
enough about the rest of the cases.

So, from my side, select either approach and check it in. We can always
revisit this later. I'd personally prefer a very simple FileCheck in the
extra tools repo.

Cheers,
/Manuel


>
> On Mon, Jan 14, 2013 at 12:41 PM, Alexander Kornienko <alexfh at google.com>wrote:
>>
>>> On Mon, Jan 14, 2013 at 12:13 PM, Alexander Kornienko <alexfh at google.com
>>> > wrote:
>>>
>>>> In this case I'd prefer to use VerifyDiagnosticConsumer, which is an
>>>> implementation of clang's "-verify" option. Any concerns?
>>>>
>>>
>>> I found a little problem with both FileCheck and "-verify" approaches:
>>> they both require a stand-alone binary, which it resides in
>>> clang-tools-extra.
>>>
>>>
>>>
>>>>
>>>>
>>>> On Mon, Jan 14, 2013 at 11:26 AM, Daniel Jasper <djasper at google.com>wrote:
>>>>
>>>>> +1, seems like the better approach.
>>>>>
>>>>>
>>>>> On Mon, Jan 14, 2013 at 11:18 AM, Manuel Klimek <klimek at google.com>wrote:
>>>>>
>>>>>>
>>>>>>   This still seems like an awful lot of code for checking 2(!)
>>>>>> diagnostics. I'd actually vote for using FileCheck.
>>>>>>
>>>>>> http://llvm-reviews.chandlerc.com/D290
>>>>>>
>>>>>> BRANCH
>>>>>>   svn
>>>>>>
>>>>>> ARCANIST PROJECT
>>>>>>   clang
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Alexander Kornienko | Software Engineer | alexfh at google.com | +49 151
>>>> 221 77 957
>>>> Google Germany GmbH | Dienerstr. 12 | 80331 München
>>>>
>>>
>>>
>>>
>>> --
>>> Alexander Kornienko | Software Engineer | alexfh at google.com | +49 151
>>> 221 77 957
>>> Google Germany GmbH | Dienerstr. 12 | 80331 München
>>>
>>
>>
>
>
> --
> Alexander Kornienko | Software Engineer | alexfh at google.com | +49 151 221
> 77 957
> Google Germany GmbH | Dienerstr. 12 | 80331 München
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130114/7447d71b/attachment.html>