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

Jordan Rose jordan_rose at apple.com
Mon Jul 2 10:28:56 PDT 2012

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"?

More information about the cfe-commits mailing list