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

[Andy Gibbs]

On Monday, July 02, 2012 8:54 PM, Andy Gibbs wrote:
> On Monday, July 02, 2012 7:28 PM, Jordan Rose wrote:
>> On Jun 30, 2012, at 2:12 PM, Andy Gibbs wrote:
>>
>> Translating the expected line number in and out of a SourceLocation seems
>> silly to me, especially because this isn't free. Is there any problem 
>> with
>> just keeping an 'unsigned ExpectedLine' around?
>
> The reason for doing this was because early on Richard asked for the 
> ability
> to include a file name match [...snip...] Plus you'll see in the 
> diagnostics
> that the filename is displayed where an expected-* check is not matched to
> a diagnostic. [...snip...]

The attached patch leaves this unchanged, therefore, pending the 
implementation
of this additional feature at the next stage.

>> I see we do check if the location is invalid; I think it's okay to make
>> that an assertion. These checks should never come from or refer to lines 
>> on
>> the command line.

I'm afraid it isn't possible to do an assertion since it is very possible 
for
a test-case writer to provide an invalid line number, for example:

// expected-error@ {{...}}
// expected-error at 0 {{...}}
// expected-error at -3 {{...}}

(the third will be invalid if the line number of the directive is 3 or 
less).

>> +  StandardDirective(const SourceLocation &DirectiveLoc,
>> +                    const SourceLocation &DiagnosticLoc,
>> +                    const std::string &Text, unsigned Count)
>> +    : Directive(DirectiveLoc, DiagnosticLoc, Text, Count) { }
>>
>> StringRef. :-)

Done! :o)

>> +        unsigned CurrentLine = SM.getSpellingLineNumber(Pos, &Invalid);
>> +        if (!Invalid && PH.Next(Line) && (FoundPlus || Line <
>> CurrentLine)) {
>> +          if (FoundPlus) CurrentLine += Line;
>> +          else CurrentLine -= Line;
>> +          ExpectedLoc = SM.translateLineCol(SM.getFileID(Pos),
>> CurrentLine, 1);
>> +        }
>>
>> Nitpick: modifying something called "CurrentLine" confused me for a
>> second,
>> even though it's obvious what you're doing. "ExpectedLine"?
>
> I don't mind nitpicks -- I'll change this too...

And I have!

So attached is the new "part 2" patch.  Ready for commit?

Thanks
Andy

 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: verify-part2.diff
Type: application/octet-stream
Size: 12334 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120704/8d5875c1/attachment.obj>