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

[Andy Gibbs]

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, e.g. of the form:

expected-error at file:line {{ ... }}

meaning that storing a SourceLocation made more sense to me than a 
filename/line
number pair.  Plus you'll see in the diagnostics that the filename is 
displayed where
an expected-* check is not matched to a diagnostic.  This means that an 
expected-*
check can be in a different file (which, btw, is already the case in a 
number of test-
cases) and the diagnostic output from -verify will inform the coder where 
the
expected-* directive is held (which may not be the same file as the 
diagnostic
occurs in).   At the point where we do actually support the full file:line 
syntax, a
number of these test-cases will need to be amended to handle the fact that 
they
use a directive in one file to match a diagnostic in another... (but that's 
a story for
another day).

> 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'll have a look into this tomorrow when I'm in front of the code again.

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

No problems! (and also in your patch 1 comment too).

> +        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...

Thanks
Andy