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

[Andy Gibbs]

On Tuesday, July 03, 2012 5:18 PM, Jordan Rose wrote:
> On Jul 2, 2012, at 10:39 PM, Andy Gibbs wrote:
> [...snip...]
>> 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.)

Hmm, I'm not sure I understand enough about how the modules part of the 
compiler
works to change a test-case so drastically.  Would it not be better to get 
this
changed by someone better qualified?

> 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.

I'd certainly like to add this feature since I believe it to have merit, but
my timescale would be the latter end of the month or beginning of August to
be able to devote the time to it.

> 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.

Really, no problem -- its very difficult to keep track of all the 
discussions
that have gone on regarding this patch!  Even I forget some things!

> 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.

I totally agree (as I have mentioned elsewhere) and this is the reasoning 
behind
the contentious part 6 patch -- it removes one source of "unseen" files. 
There
are two others, of which I have pinned one down but I don't have a patch I 
am
pleased with yet.  As I said earlier, I have to push this work out to later 
in
July/August since I am about to become a father for the second time and this
will call me into other duties for a short while.

> 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.

Thank you: high praise makes it all worth while.  I, for my part, am very
grateful that you (and Richard) have taken the time to diligently critique
my patches.

I have just posted the first three patches with the latest round of comments
implemented, and I trust are now commonly held to be acceptable.  Would it 
be
possible to get these actually committed now?  I guess you'd like all the
patches to be ready together, but we're still discussing patch 4 and 6, in
particular, and I would like to be able to get the first three patches out 
of
the way so that I don't have to keep rebasing them...  The changes they make
are complete in themselves and cause no test regressions.  I hope it isn't 
too
unreasonable a request?

Regarding the remaining patches, this is my suggestion:

Patch 4: I have an alternative proposal for this which approaches the 
problem
from another angle.  I think it still may raise the same concerns, but I 
will
try to post a patch to it shortly to see what you think.  If you think not,
then we can leave this patch out and I will adjust the subsequent patches to
fit.  I will have to retain this patch in the separate distribution I 
maintain
since I have users requesting this fix.

Patch 5: This will get adjusted as regards the position with patch 4.

Patch 6: I am happy to leave this out altogether at present.  As mentioned
above, ultimately I think we should put some solution in as this will move
towards removing the "unseen" files problem.  I have in mind what may be a
better solution... watch this space!

Patch 7: This again will need to be adjusted to fit the position regarding
patches 4 and 6 since there is a test-case for each of these patches
contained within.

Thanks again,

Andy