[cfe-commits] [Patch 7 of 7] -verify fixes and enhancement
andyg1001 at hotmail.co.uk
Tue Jul 3 15:23:06 PDT 2012
On Tuesday, July 03, 2012 5:18 PM, Jordan Rose wrote:
> On Jul 2, 2012, at 10:39 PM, Andy Gibbs wrote:
>> 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
>> 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
> is exercised anywhere else, though. Perhaps it should be put into a test
> 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
works to change a test-case so drastically. Would it not be better to get
changed by someone better qualified?
> On the other hand, I can see an argument for wanting -verify checks to
> appear in the main source file, but for that we should probably wait and
> 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.:
> *facepalm* Should have noticed they were all PCH. Thanks, Andy.
Really, no problem -- its very difficult to keep track of all the
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
> 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
the contentious part 6 patch -- it removes one source of "unseen" files.
are two others, of which I have pinned one down but I don't have a patch I
pleased with yet. As I said earlier, I have to push this work out to later
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
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
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
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
unreasonable a request?
Regarding the remaining patches, this is my suggestion:
Patch 4: I have an alternative proposal for this which approaches the
from another angle. I think it still may raise the same concerns, but I
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
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
More information about the cfe-commits