On Jul 5, 2012, at 0:18 , Andy Gibbs <[email protected]> wrote:
> On Wednesday, July 04, 2012 5:41 PM, Andy Gibbs wrote: >> On Wednesday, July 04, 2012 2:35 AM, Jordan Rose wrote: >>> [...snip...] >>> Oops, I see that's already been taken care of. Wait, so why do >>> we need the additional check at the end? Referring to this part >>> of the patch: >>> >>>> - if (D.Location.isInvalid() || !SourceMgr) >>>> + if (D.DiagnosticLoc.isInvalid() || !SourceMgr) >>> >>> I see that it's not /your/ isInvalid() originally, but I don't >>> think anything should break if you take it away. ...probably. >> >> Well it passed the simple check which was to stick an assert in >> and see if anything broke! I also cannot conceive of a reason >> why DiagnosticLoc could be invalid at this point. >> >> However, I would be tempted to leave it in since it was in before. >> I can only think that the originator had some reason for making >> the check rather than asserting, meaning that he may have foreseen >> a situation where DiagnosticLoc may be invalid. I will check >> through the commit logs to see if anything comes up. > > I have checked through the commits and I believe that the isInvalid > check comes from r102516 when the PrintProblem function was > duplicated. I believe it is safe therefore to remove the check in > this duplicated version of PrintProblem since it is only used in > the situation where DiagnosticLoc must be valid. > > Attached then is a patch which tidies up the PrintProblem functions. > It renames them PrintUnexpected and PrintExpected according to their > application. The SourceManager pointer in PrintExpected has been > changed to a reference and the "if" clause has subsequently dropped > out. > > A similar change can not be made to PrintUnexpected since it may be > used in a situation either where there is not a SourceManager or > where the diagnostic location is invalid. > > This patch is in addition to patches 1, 2 and 3 posted recently. > It should be possible to apply it directly after patch 2, but I > have only tested it applied after patch 3. > > Cheers, > Andy > <verify-part2a.diff> Looks good to me! One thing that could make it even better: don't print the directive location if it's on the same line as the diagnostic location. Jordan _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
