On Monday, October 15, 2012 6:43 PM, David Blaikie wrote:
On Mon, Oct 15, 2012 at 8:53 AM, Andy Gibbs <[email protected]> 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
part3-v2.diff
Description: Binary data
part1-v2.diff
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
