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

[Andy Gibbs]

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: part1.diff
Type: application/octet-stream
Size: 9055 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121011/cc93f5b1/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: part3.diff
Type: application/octet-stream
Size: 11915 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121011/cc93f5b1/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: part2.diff
Type: application/octet-stream
Size: 266132 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121011/cc93f5b1/attachment-0002.obj>