On Mon, Oct 15, 2012 at 1:39 PM, Andy Gibbs <[email protected]> wrote: > 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.
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)) _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
