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