On Jun 30, 2012, at 2:12 PM, Andy Gibbs wrote:
> Patch 2: reworked the directive parsing logic as per Richard's suggestion. It
> also now includes the directive location as well as the expected diagnostic
> location (both then are displayed when a directive is not matched). I also
> fixed a few comments which weren't capitalised in the original code since
> this was also raised. I'm afraid I haven't gone through all the clang code,
> but maybe these few will be a start! :o)
>
> <verify-part2.diff>
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?
(We'll have to build the line number table no matter what, but lookups into it
use a binary search with cache, IIRC.)
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.
+ StandardDirective(const SourceLocation &DirectiveLoc,
+ const SourceLocation &DiagnosticLoc,
+ const std::string &Text, unsigned Count)
+ : Directive(DirectiveLoc, DiagnosticLoc, Text, Count) { }
StringRef. :-)
+ 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"?
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
