[cfe-commits] [PATCH] expected-no-diagnostics for -verify (was: Re: r164677 - )
dblaikie at gmail.com
Mon Oct 15 13:46:38 PDT 2012
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>
>>> Attached is an cumulative update to part 2 of the patch previously
>>> 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
>> 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
> 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.
>> + || 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))
More information about the cfe-commits