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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
