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

[Jordan Rose]

On Jul 5, 2012, at 0:18 , Andy Gibbs <andyg1001 at hotmail.co.uk> 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