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

[Andy Gibbs]

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.

Many thanks again,

Andy


-------------- next part --------------
A non-text attachment was scrubbed...
Name: part3-v2.diff
Type: application/octet-stream
Size: 14165 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121015/119e5265/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: part1-v2.diff
Type: application/octet-stream
Size: 9093 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121015/119e5265/attachment-0001.obj>