[cfe-commits] [Patch 7 of 7] -verify fixes and enhancement

[Jordan Rose]

On Jul 2, 2012, at 10:39 PM, Andy Gibbs wrote:

> On Tuesday, July 03, 2012 2:33 AM, Jordan Rose wrote:
>>> Index: tools/clang/test/Modules/Inputs/Module.framework/Headers/Module.h
>>> ==================================================================
>>> --- tools/clang/test/Modules/Inputs/Module.framework/Headers/Module.h
>>> +++ tools/clang/test/Modules/Inputs/Module.framework/Headers/Module.h
>>> @@ -1,6 +1,6 @@
>>> -// expected-warning{{umbrella header}}
>>> +// expected-warning 0-1 {{umbrella header}}
>>> 
>>> #ifndef MODULE_H
>>> #define MODULE_H
>>> const char *getModuleVersion(void);
>> 
>> This does not mean the same thing. Why is it being changed?
> 
> You are right, it means the expected warning may or may not appear once.
> What you observe here is a test-case bug which was exposed by handling
> included files properly once my patch is applied.  I imagine that if proper
> handling of include files was always the case, this test-case would have
> been written differently...
> 
> Let me give an example: (assuming my patch isn't applied)
> 
> ///// test-case-include.h
> // expected-warning{{some warning}}
> 
> ///// test-case.c
> #include "test-case-include.h"
> 
> What would you expect if you ran the test-case with -verify?  I should
> expect that it fails since there is an unseen warning, however, it will
> pass.  This is one of the bugs in -verify which is now fixed.
> 
> In the case above, Module.h is shared across a number of tests.  In some
> tests the include file was parsed correctly and in others it was not.  (I
> made some comments about a net with holes in another post and this
> is one example of where it applied!)  Unfortunately, this incorrect parsing
> coincided with the cases where the diagnostic also not generated (if you
> look at the original implementation you will understand why), so the
> test-case bug was missed.  Since the diagnostic sometimes is and
> sometimes is not generated, hence the "0-1".

I see. It doesn't look like the "umbrella header" warning (-Wincomplete-umbrella) is exercised anywhere else, though. Perhaps it should be put into a test of its own? (I think it's reasonable to make a separate "Umbrella.framework" because of the existing expectation in Module.framework.)

On the other hand, I can see an argument for wanting -verify checks to only appear in the main source file, but for that we should probably wait and see about adding a file:line mode.

Thanks for the explanation.


>> In general I think the "referenced here" / "declared here" notes don't need to be
>> moved. Sure, going forward we should be writing them using @line, but I think
>> it's okay to leave existing ones as is. Although I would actually expect there to be
>> many more such notes, so were you already only doing a few of them? How
>> were they being chosen?
> 
> You mean the PCH test-cases?  Well in this case, as you know, the PCH doesn't
> include comments within it once it is generated, hence these comments must
> be moved out of the PCH declaration into the actual test-case -- i.e. out of the
> #if/#endif that is used for the PCH generation.
> 
> Can I refer you to my submission post which, hopefully, explains a lot of the
> rationale behind the patch, the necessary test-case changes, the bugs fixed, etc.:
> 
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120611/058902.html

*facepalm* Should have noticed they were all PCH. Thanks, Andy.

As I said before (without context) I don't like the idea of reparsing "unseen" files at the end; I think our implementation of CommentHandler and VerifyDiagnosticsHandler should just be good enough to handle these cases, and/or that -verify doesn't support expectations in header files.

In general, though, I think this is a very useful cleanup/enhancement to -verify, and although I think these policy issues should be resolved (and one more person should probably review the patches) I'm hoping they can go in soon.

Thanks again!
Jordan