<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jul 23, 2012, at 14:10 , Andy Gibbs <<a href="mailto:andyg1001@hotmail.co.uk">andyg1001@hotmail.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">On Friday, July 20, 2012 8:59 PM, Andy Gibbs wrote:<br><blockquote type="cite">On Friday, July 20, 2012 6:04 PM, Jordan Rose wrote:<br><blockquote type="cite">On Jul 17, 2012, at 9:18 AM, Andy Gibbs wrote:<br><blockquote type="cite">4. Ensure that repeated BeginSourceFile calls are handled correctly, as <br>is<br> a mismatch between calls of BeginSourceFile and EndSourceFile (it turns<br> out, the latter is often called twice while the former only once, but<br> also it is possible for BeginSourceFile to be called more than once <br>too).<br></blockquote><br>This worries me. If we have unbalanced BeginSourceFile and EndSourceFile<br>calls, that might be a bug. Do you know specific circumstances where this<br>is the case?<br></blockquote><br>Not off the top of my head. I'm afraid I'm at home now, so don't have the<br>code in front of me, plus it was a week ago that I was doing this patch.<br>If I was going to have a guess, I would hazard that it was inside<br>FrontendAction::EndSourceFile(), but it would be much better that I<br>go away and look it up rather than guess!...<br></blockquote><br>No, I was wrong. It is FrontendActciont::BeginSourceFile where the<br>problem exists. What happens is there is a "goto failure" error handler<br>which calls "EndSourceFile" even if "BeginSourceFile" hasn't yet been<br>called. Below is a proposed patch.<br><br>I can then change my previous patch to assert on mismatch of BeginSourceFile<br>and EndSourceFile. Were you happy with my other responses to your<br>reservations? If you, shall I now post a revised patch incorporating all<br>those changes?<br></blockquote><div><br></div><div>You missed a few BeginSourceFile calls, but the basic approach seems right. And I really wouldn't track the mismatch of BeginSourceFile and EndSourceFile—it's not really VerifyDiagnosticConsumer's job. But I like this fix a lot—we should have been doing the right thing all along.</div><div><br></div><div><br></div><div>Back to your first replies...</div><div><br></div><div></div><blockquote type="cite"><div><span style="font-family: monospace; ">The point I'm trying to make (albeit badly!) is that the problem is more</span><br style="font-family: monospace; "><span style="font-family: monospace; ">likely during the parse stage rather than in the post-processing stage</span><br style="font-family: monospace; "><span style="font-family: monospace; ">and as a result, a solution should be sought elsewhere first before making</span><br style="font-family: monospace; "><span style="font-family: monospace; ">changes to this part of the code.</span><br style="font-family: monospace; "><br style="font-family: monospace; "><span style="font-family: monospace; ">How about:</span><br style="font-family: monospace; "><br style="font-family: monospace; "><span style="font-family: monospace; ">// In a debug build, scan through any files that may have been missed</span><br style="font-family: monospace; "><span style="font-family: monospace; ">// during parsing and issue a fatal error if directives are contained</span><br style="font-family: monospace; "><span style="font-family: monospace; ">// within these files. If a fatal error occurs, this suggests that</span><br style="font-family: monospace; "><span style="font-family: monospace; ">// this file is being parsed separately from the main file, for example</span><br style="font-family: monospace; "><span style="font-family: monospace; ">// it is a precompiled header. To avoid the fatal error, the comments</span><br style="font-family: monospace; "><span style="font-family: monospace; ">// from these files should preferably be extracted at source and then</span><br style="font-family: monospace; "><span style="font-family: monospace; ">// forwarded directly to VerifyDiagnosticConsumer for proper processing.</span><br style="font-family: monospace; "><span style="font-family: monospace; ">// If this is not appropriate, then as a fallback the files should be</span><br style="font-family: monospace; "><span style="font-family: monospace; ">// explicitly ignored here.</span><br style="font-family: monospace; "></div></blockquote><div><br></div><div>Better, but still confusing. I would actually just cut everything after "for example [if] it is a precompiled header". That's probably enough to help anyone looking at this code later, and people who make the mistake probably won't look here right away to see what's going on. And after all, the usual fix isn't to forward the comments, it's to move the directives to the main source file.</div><div><br></div><div><br></div><div><blockquote type="cite" style="font-family: monospace; "></blockquote><blockquote type="cite"><blockquote type="cite" style="font-family: monospace; ">+ if (FoundExpectedDiags(*CurrentPreprocessor, *I))<br>+ llvm::report_fatal_error(Twine("-verify directives found after rather"<br>+ " than during normal parsing of ",<br>+ StringRef(E ? E->getName() : "(unknown)")));<br><br>Is it possible to get a test case for this, or is it really impossible?<br>If the latter, you might as well just<br>assert(!FoundExpectedDiags(*CurrentPreprocessor, *I)).<br></blockquote><br style="font-family: monospace; "><span style="font-family: monospace; ">If you just apply the first patch and none of the latter patches, then</span><br style="font-family: monospace; "><span style="font-family: monospace; ">yes you will hit this fatal error. Once all the holes are patched, you</span><br style="font-family: monospace; "><span style="font-family: monospace; ">shouldn't expect to see it until someone adds a new feature like PCH or</span><br style="font-family: monospace; "><span style="font-family: monospace; ">modules! The reason I went for report_fatal_error over assert was because</span><br style="font-family: monospace; "><span style="font-family: monospace; ">then you can get the very informative error which tells you exactly which</span><br style="font-family: monospace; "><span style="font-family: monospace; ">source file has caused the error. I think this has merit.</span><br style="font-family: monospace; "></blockquote></div><div><br></div><div>Okay, makes sense.</div><div><br></div><div>Please do send a revised patch now! And thanks, as always.</div><div>Jordan</div></div><br></body></html>