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

[Andy Gibbs]

Hi,

Attached is an cumulative update to part 2 of the patch previously posted, 
updating three more tests.

Are we good to commit yet?

Cheers
Andy


On Thursday, October 11, 2012 7:35 PM, Andy Gibbs wrote:
> Hi,
>
> Thanks for the feedback.  Attached then is the patch split three ways:
>
> Part 1 is an additional patch that fixes a "bug" whereby
> VerifyDiagnosticConsumer would parse this as a valid directive:
> "junkexpected-error {{...}}" which I think most people would not expect!
> The fix ensures that "expected-..." is the beginning of the word, or 
> follows
> immediately from the "//" or "/*" of a comment.  In doing the fix, a 
> couple
> of test-cases had to be adjusted too.
>
> Part 2 is all the test-case changes incorporating the new
> "expected-no-diagnostics" directive.
>
> Part 3 is the change to VerifyDiagnosticConsumer and the remaining
> test-cases.
>
> I've tried to make the patches as close to current as possible, but new
> commits keep bringing in new test-cases that need the new directive, so it
> is possible it is already behind by the time you read this!
>
> Responses to your comments are inline below.
>
> Many thanks
>
> Andy
>
>
> On Wednesday, October 10, 2012 7:17 PM, David Blaikie wrote:
>> looks fairly reasonable, though I'd expect Doug or someone to sign off
>> on a new feature(tte) like this
>>
>> Some nits/thoughts:
>> * VerifyDiagnosticConsumer.cpp: braced one line else (usually we drop
>> the braces from one line blocks) around "Status =
>> ...HasExpectedNoDiagnostics"
>
> Done.
>
>> * verify3.c:
>>  * using -D to separate multiple tests within a single file can be a
>> bit hard to follow. Would this be possible/easier to read if they were
>> just separate files?
>
> IMHO its better to keep what is effectively variations on the one test in
> one place, but if you really want it split up I can do this, of course!
>
>>  * Also the right-alignment of CHECK prefixes isn't common, though
>> not inherently problematic. (just seems like it'd make for some
>> maintenance difficulty in the future should any check prefix become
>> longer than the longest one already in use)
>
> Again, IMHO I think it is easier to read right-aligned: but they're not 
> all
> right aligned with each other, just those in the current block.
>
>> * It might be nice to commit the expected-no-diagnostics fixes first,
>> then commit the improvement just to separate the mechanical from the
>> interesting work.
>
> Done!
>
>> * verify.m: is there a reason you didn't use expected-no-diagnostics
>> there?
>
> Yes, its to check that the changes work through the ARCMT "re-direction"
> layer.
>
>> On Wed, Oct 10, 2012 at 1:21 AM, Andy Gibbs> wrote:
>>> ** bump ** :o)
>>>
>>> I've had to resync and attach the patch again here; a test-case needed 
>>> to
>>> be
>>> updated to reflect changes in trunk.  The patch now is taken against
>>> r165609.
>>>
>>> Cheers
>>> Andy
>>>
>>>
>>> On Saturday, October 06, 2012 10:59 PM, Andy Gibbs wrote:
>>>>
>>>> Hi,
>>>>
>>>> Was there any interest in this patch?  Please let me know if there are
>>>> any
>>>> changes that should be made, or whether it is good to be committed? (or
>>>> even
>>>> whether I was barking up the wrong tree...?!)
>>>>
>>>> (If any were unable to receive the patch -- I know that sometimes Apple
>>>> users have problems with my attachments! -- then it can be viewed
>>>> directly
>>>> at [...snip...])
>>>>
>>>> Cheers
>>>> Andy
>>>>
>>>>
>>>> On Tuesday, October 02, 2012 5:32 PM, Andy Gibbs wrote:
>>>>>
>>>>> All,
>>>>>
>>>>> Attached is a patch that adds an 'expected-no-diagnostics' directive
>>>>> to -verify.  This can then be used, as discussed, to flag files that
>>>>> are
>>>>> not
>>>>> expected to produce any diagnostics at all.  -verify has then been
>>>>> changed
>>>>> to generate an error if no expected-* directives are found (for
>>>>> example,
>>>>> if
>>>>> the RUN line doesn't specify the source file!).  I've also made it so
>>>>> that
>>>>> the user cannot put 'expected-no-diagnostics' in with other expected-*
>>>>> directives (to avoid mistakes!).
>>>>>
>>>>> The complete patch is rather large, but I've grouped the changes to
>>>>> VerifyDiagnosticConsumer to the top of the file along with the
>>>>> additional
>>>>> test-case.  The rest of the patch makes the necessary changes to fix
>>>>> 580
>>>>> test-cases which now (in almost all cases) need to have the
>>>>> 'expected-no-diagnostics' directive.
>>>>>
>>>>> Comments / questions welcome!
>>>>>
>>>>> Andy
>>
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: verify-update.diff
Type: application/octet-stream
Size: 1261 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121015/c0493357/attachment.obj>