[cfe-commits] [PATCH] expected-no-diagnostics for -verify (was: Re: r164677 - )

[David Blaikie]

On Mon, Oct 15, 2012 at 1:39 PM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:
> On Monday, October 15, 2012 6:43 PM, David Blaikie wrote:
>>
>> On Mon, Oct 15, 2012 at 8:53 AM, Andy Gibbs <andyg1001 at hotmail.co.uk>
>> wrote:
>>>
>>> Hi,
>>>
>>> Attached is an cumulative update to part 2 of the patch previously
>>> posted,
>>> updating three more tests.
>>
>>
>> Thanks for pinging/updating.
>>
>>> Are we good to commit yet?
>>
>>
>> I was hoping others might weigh in on both these patches (patch1 and
>> patch3).
>>
>> patch1 I'm on the fence about because the "un" prefix does make things
>> quite legible, and all but, arguably, the '-' prefix weren't bugs or
>> confusions by the looks of it. And I'd consider using "FIXME: "
>> instead of "unexpected: " if we're not going to have
>> "unexpected-error/warning/note".
>
>
> In the manner of all things, I discovered this by making a typo myself and
> discovering to my surprise that -verify still detected the directive.  I
> felt it to be unexpected behaviour.  I think if we want to have
> "unexpected-..." then a special directive should be created that emphasises
> that it is not an "expected-..." directive.
>
>
>> patch3 is probably the right thing to do, as using -Werror does change
>> the behavior of a test which might specifically want to test a
>> diagnostic in a warning, not an error, state.
>>
>> I'll sign off on all 3 so you don't have to keep fixing this up, but
>> please email cfe-dev and, possibly, document this somewhere (though I
>> can't find anywhere that the -verify syntax is documented)
>
>
> I think it is provided pretty adequately via the doxygen web pages.  I've
> added a link to this in the Clang Internals web-page, which is the only
> place (that I can find!) that mentions -verify.  As a result, I've also
> tidied up the doxygen documentation.
>
>
> And later on Monday, October 15, 2012 6:47 PM, David Blaikie again wrote:
>>
>> Oh, a few minor actual code comments:
>>
>> +      if (Status == VerifyDiagnosticConsumer::HasOtherExpectedDirectives)
>> {
>> +        Diags.Report(Pos, diag::err_verify_invalid_no_diags)
>> +          << /*IsExpectedNoDiagnostics=*/true;
>> +      } else
>>
>> We usually skip braces on single statement blocks.
>
>
> Done!
>
>
>> +            || P == Begin || isspace(P[-1])
>> +            // Or it could be preceeded by the start of a comment.
>> +            || ((P[-1] == '/' || P[-1] == '*') && P[-2] == '/'))
>>
>> Wouldn't the P[-2] possibly underrun if P == Begin + 1?
>
>
> Technically not since "Begin" points to the beginning of the comment line
> which includes the /* or //.  But stylistically you are right.  I've fixed
> this also in the attached patch.
>
> Attached are replacement part 1 and 3 patches.

Looks good, please commit. (for future reference: my previous approval
was sufficient for you to commit these even though you'd made changes
(they were changes in response to my comments anyway))