Very nice! One comment though, it seems like it might be nice to give some indication that the line is truncated. Leading and trailing ellipsis, perhaps?<br><br> - Daniel<br><br><div class="gmail_quote">On Fri, May 1, 2009 at 4:32 PM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Author: dgregor<br>
Date: Fri May 1 18:32:58 2009<br>
New Revision: 70597<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=70597&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=70597&view=rev</a><br>
Log:<br>
When printing a source line as part of a diagnostic, the source line<br>
might be wider than we're supposed to print. In this case, we try to<br>
select the "important" subregion of the source line, which contains<br>
everything that we want to show (e.g., with underlining and the caret<br>
itself) and tries to also contain some of the context.<br>
<br>
>From the fantastically long line in the test case, we get an error<br>
message that slices down to this:<br>
<br>
message-length.c:18:120: warning: comparison of distinct pointer types<br>
('int *' and 'float *')<br>
a_func_to_call(ip == FloatPointer, ip[ALongIndexName],<br>
~~ ^ ~~~~~~~~~~~~<br>
<br>
There are a bunch of gee-it-sounds-good heuristics in here, which seem<br>
to do well on the various simple tests I've thrown at it. However,<br>
we're going to need to look at a bunch more diagnostics to tweak these<br>
heuristics.<br>
<br>
This is the second part of <rdar://problem/6711348>. Almost there!<br>
<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Frontend/TextDiagnosticPrinter.h<br>
cfe/trunk/lib/Frontend/TextDiagnosticPrinter.cpp<br>
cfe/trunk/test/Misc/message-length.c<br>
<br>
Modified: cfe/trunk/include/clang/Frontend/TextDiagnosticPrinter.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/TextDiagnosticPrinter.h?rev=70597&r1=70596&r2=70597&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/TextDiagnosticPrinter.h?rev=70597&r1=70596&r2=70597&view=diff</a><br>
<br>
==============================================================================<br>
--- cfe/trunk/include/clang/Frontend/TextDiagnosticPrinter.h (original)<br>
+++ cfe/trunk/include/clang/Frontend/TextDiagnosticPrinter.h Fri May 1 18:32:58 2009<br>
@@ -74,7 +74,8 @@<br>
SourceManager &SM,<br>
const CodeModificationHint *Hints,<br>
unsigned NumHints,<br>
- unsigned AvoidColumn);<br>
+ unsigned AvoidColumn,<br>
+ unsigned Columns);<br>
<br>
virtual void HandleDiagnostic(Diagnostic::Level DiagLevel,<br>
const DiagnosticInfo &Info);<br>
<br>
Modified: cfe/trunk/lib/Frontend/TextDiagnosticPrinter.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/TextDiagnosticPrinter.cpp?rev=70597&r1=70596&r2=70597&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/TextDiagnosticPrinter.cpp?rev=70597&r1=70596&r2=70597&view=diff</a><br>
<br>
==============================================================================<br>
--- cfe/trunk/lib/Frontend/TextDiagnosticPrinter.cpp (original)<br>
+++ cfe/trunk/lib/Frontend/TextDiagnosticPrinter.cpp Fri May 1 18:32:58 2009<br>
@@ -108,20 +108,158 @@<br>
CaretLine[i] = '~';<br>
}<br>
<br>
+/// \brief Whether this is a closing delimiter such as ')' or ']'.<br>
+static inline bool isClosingDelimiter(char c) {<br>
+ return c == ')' || c == ']' || c == '}';<br>
+}<br>
+<br>
+/// \brief When the source code line we want to print is too long for<br>
+/// the terminal, select the "interesting" region.<br>
+static void SelectInterestingSourceRegion(std::string &SourceLine,<br>
+ std::string &CaretLine,<br>
+ std::string &FixItInsertionLine,<br>
+ unsigned Columns) {<br>
+ if (CaretLine.size() > SourceLine.size())<br>
+ SourceLine.resize(CaretLine.size(), ' ');<br>
+<br>
+ // Find the slice that we need to display the full caret line<br>
+ // correctly.<br>
+ unsigned CaretStart = 0, CaretEnd = CaretLine.size();<br>
+ for (; CaretStart != CaretEnd; ++CaretStart)<br>
+ if (!isspace(CaretLine[CaretStart]))<br>
+ break;<br>
+<br>
+ for (; CaretEnd != CaretStart; --CaretEnd)<br>
+ if (!isspace(CaretLine[CaretEnd - 1]))<br>
+ break;<br>
+<br>
+ // CaretLine[CaretStart, CaretEnd) contains all of the interesting<br>
+ // parts of the caret line. While this slice is smaller than the<br>
+ // number of columns we have, try to grow the slice to encompass<br>
+ // more context.<br>
+<br>
+ // If the end of the interesting region comes before we run out of<br>
+ // space in the terminal, start at the beginning of the line.<br>
+ if (CaretEnd < Columns)<br>
+ CaretStart = 0;<br>
+<br>
+ unsigned TargetColumns = Columns - 4; // Give us a little extra room.<br>
+ unsigned SourceLength = SourceLine.size();<br>
+ bool StartIsFixed = false;<br>
+ while (CaretEnd - CaretStart < TargetColumns) {<br>
+ bool ExpandedRegion = false;<br>
+ // Move the start of the interesting region left until we've<br>
+ // pulled in something else interesting.<br>
+ if (CaretStart && !StartIsFixed &&<br>
+ CaretEnd - CaretStart < TargetColumns) {<br>
+ unsigned NewStart = CaretStart;<br>
+<br>
+ bool BadStart = false;<br>
+ do {<br>
+ // Skip over any whitespace we see here; we're looking for<br>
+ // another bit of interesting text.<br>
+ if (NewStart)<br>
+ --NewStart;<br>
+ while (NewStart && isspace(SourceLine[NewStart]))<br>
+ --NewStart;<br>
+<br>
+ // Skip over this bit of "interesting" text.<br>
+ while (NewStart && !isspace(SourceLine[NewStart])) {<br>
+ if (isClosingDelimiter(SourceLine[NewStart]))<br>
+ StartIsFixed = true;<br>
+ --NewStart;<br>
+ }<br>
+<br>
+ // Move up to the non-whitespace character we just saw.<br>
+ if (!StartIsFixed &&<br>
+ isspace(SourceLine[NewStart]) &&<br>
+ !isspace(SourceLine[NewStart + 1]))<br>
+ ++NewStart;<br>
+<br>
+ // Never go back past closing delimeters, because<br>
+ // they're unlikely to be important (and they result in<br>
+ // weird slices). Instead, move forward to the next<br>
+ // non-whitespace character.<br>
+ BadStart = false;<br>
+ if (StartIsFixed) {<br>
+ ++NewStart;<br>
+ while (NewStart != CaretEnd && isspace(SourceLine[NewStart]))<br>
+ ++NewStart;<br>
+ } else if (NewStart) {<br>
+ // There are some characters that always signal that we've<br>
+ // found a bad stopping place, because they always occur in<br>
+ // the middle of or at the end of an expression. In these<br>
+ // cases, we either keep bringing in more "interesting" text<br>
+ // to try to get to a somewhat-complete slice of the code.<br>
+ BadStart = ispunct(SourceLine[NewStart]);<br>
+ }<br>
+ } while (BadStart);<br>
+<br>
+ // If we're still within our limit, update the starting<br>
+ // position within the source/caret line.<br>
+ if (CaretEnd - NewStart <= TargetColumns && !StartIsFixed) {<br>
+ CaretStart = NewStart;<br>
+ ExpandedRegion = true;<br>
+ }<br>
+ }<br>
+<br>
+ // Move the end of the interesting region right until we've<br>
+ // pulled in something else interesting.<br>
+ if (CaretEnd != SourceLength &&<br>
+ CaretEnd - CaretStart < TargetColumns) {<br>
+ unsigned NewEnd = CaretEnd;<br>
+<br>
+ // Skip over any whitespace we see here; we're looking for<br>
+ // another bit of interesting text.<br>
+ while (CaretEnd != SourceLength && isspace(SourceLine[NewEnd - 1]))<br>
+ ++NewEnd;<br>
+<br>
+ // Skip over this bit of "interesting" text.<br>
+ while (CaretEnd != SourceLength && !isspace(SourceLine[NewEnd - 1]))<br>
+ ++NewEnd;<br>
+<br>
+ if (NewEnd - CaretStart <= TargetColumns) {<br>
+ CaretEnd = NewEnd;<br>
+ ExpandedRegion = true;<br>
+ }<br>
+<br>
+ if (!ExpandedRegion)<br>
+ break;<br>
+ }<br>
+ }<br>
+<br>
+ // [CaretStart, CaretEnd) is the slice we want. Update the various<br>
+ // output lines to show only this slice, with two-space padding<br>
+ // before the lines so that it looks nicer.<br>
+ SourceLine.erase(CaretEnd, std::string::npos);<br>
+ CaretLine.erase(CaretEnd, std::string::npos);<br>
+ if (FixItInsertionLine.size() > CaretEnd)<br>
+ FixItInsertionLine.erase(CaretEnd, std::string::npos);<br>
+<br>
+ if (CaretStart > 2) {<br>
+ SourceLine.replace(0, CaretStart, " ");<br>
+ CaretLine.replace(0, CaretStart, " ");<br>
+ if (FixItInsertionLine.size() >= CaretStart)<br>
+ FixItInsertionLine.replace(0, CaretStart, " ");<br>
+ }<br>
+}<br>
+<br>
void TextDiagnosticPrinter::EmitCaretDiagnostic(SourceLocation Loc,<br>
SourceRange *Ranges,<br>
unsigned NumRanges,<br>
SourceManager &SM,<br>
const CodeModificationHint *Hints,<br>
unsigned NumHints,<br>
- unsigned AvoidColumn) {<br>
+ unsigned AvoidColumn,<br>
+ unsigned Columns) {<br>
assert(!Loc.isInvalid() && "must have a valid source location here");<br>
<br>
// We always emit diagnostics about the instantiation points, not the spelling<br>
// points. This more closely correlates to what the user writes.<br>
if (!Loc.isFileID()) {<br>
SourceLocation OneLevelUp = SM.getImmediateInstantiationRange(Loc).first;<br>
- EmitCaretDiagnostic(OneLevelUp, Ranges, NumRanges, SM, 0, 0, AvoidColumn);<br>
+ EmitCaretDiagnostic(OneLevelUp, Ranges, NumRanges, SM, 0, 0, AvoidColumn,<br>
+ Columns);<br>
<br>
// Map the location through the macro.<br>
Loc = SM.getInstantiationLoc(SM.getImmediateSpellingLoc(Loc));<br>
@@ -210,10 +348,6 @@<br>
CaretLine.insert(i+1, NumSpaces, CaretLine[i] == '~' ? '~' : ' ');<br>
}<br>
<br>
- // Finally, remove any blank spaces from the end of CaretLine.<br>
- while (CaretLine[CaretLine.size()-1] == ' ')<br>
- CaretLine.erase(CaretLine.end()-1);<br>
-<br>
// If we are in -fdiagnostics-print-source-range-info mode, we are trying to<br>
// produce easily machine parsable output. Add a space before the source line<br>
// and the caret to make it trivial to tell the main diagnostic line from what<br>
@@ -222,27 +356,9 @@<br>
SourceLine = ' ' + SourceLine;<br>
CaretLine = ' ' + CaretLine;<br>
}<br>
-<br>
- // AvoidColumn tells us which column we should avoid when printing<br>
- // the source line. If the source line would start at or near that<br>
- // column, add another line of whitespace before printing the source<br>
- // line. Otherwise, the source line and the diagnostic text can get<br>
- // jumbled together.<br>
- unsigned StartCol = 0;<br>
- for (unsigned N = SourceLine.size(); StartCol != N; ++StartCol)<br>
- if (!isspace(SourceLine[StartCol]))<br>
- break;<br>
-<br>
- if (StartCol != SourceLine.size() &&<br>
- abs((int)StartCol - (int)AvoidColumn) <= 2)<br>
- OS << '\n';<br>
-<br>
- // Emit what we have computed.<br>
- OS << SourceLine << '\n';<br>
- OS << CaretLine << '\n';<br>
-<br>
+<br>
+ std::string FixItInsertionLine;<br>
if (NumHints && PrintFixItInfo) {<br>
- std::string InsertionLine;<br>
for (const CodeModificationHint *Hint = Hints, *LastHint = Hints + NumHints;<br>
Hint != LastHint; ++Hint) {<br>
if (Hint->InsertionLoc.isValid()) {<br>
@@ -258,19 +374,47 @@<br>
= SM.getColumnNumber(HintLocInfo.first, HintLocInfo.second);<br>
unsigned LastColumnModified<br>
= HintColNo - 1 + Hint->CodeToInsert.size();<br>
- if (LastColumnModified > InsertionLine.size())<br>
- InsertionLine.resize(LastColumnModified, ' ');<br>
+ if (LastColumnModified > FixItInsertionLine.size())<br>
+ FixItInsertionLine.resize(LastColumnModified, ' ');<br>
std::copy(Hint->CodeToInsert.begin(), Hint->CodeToInsert.end(),<br>
- InsertionLine.begin() + HintColNo - 1);<br>
+ FixItInsertionLine.begin() + HintColNo - 1);<br>
}<br>
}<br>
}<br>
+ }<br>
<br>
- if (!InsertionLine.empty()) {<br>
- if (PrintRangeInfo)<br>
- OS << ' ';<br>
- OS << InsertionLine << '\n';<br>
- }<br>
+ // If the source line is too long for our terminal, select only the<br>
+ // "interesting" source region within that line.<br>
+ if (Columns && SourceLine.size() > Columns)<br>
+ SelectInterestingSourceRegion(SourceLine, CaretLine, FixItInsertionLine,<br>
+ Columns);<br>
+<br>
+ // AvoidColumn tells us which column we should avoid when printing<br>
+ // the source line. If the source line would start at or near that<br>
+ // column, add another line of whitespace before printing the source<br>
+ // line. Otherwise, the source line and the diagnostic text can get<br>
+ // jumbled together.<br>
+ unsigned StartCol = 0;<br>
+ for (unsigned N = SourceLine.size(); StartCol != N; ++StartCol)<br>
+ if (!isspace(SourceLine[StartCol]))<br>
+ break;<br>
+<br>
+ if (StartCol != SourceLine.size() &&<br>
+ abs((int)StartCol - (int)AvoidColumn) <= 2)<br>
+ OS << '\n';<br>
+<br>
+ // Finally, remove any blank spaces from the end of CaretLine.<br>
+ while (CaretLine[CaretLine.size()-1] == ' ')<br>
+ CaretLine.erase(CaretLine.end()-1);<br>
+<br>
+ // Emit what we have computed.<br>
+ OS << SourceLine << '\n';<br>
+ OS << CaretLine << '\n';<br>
+<br>
+ if (!FixItInsertionLine.empty()) {<br>
+ if (PrintRangeInfo)<br>
+ OS << ' ';<br>
+ OS << FixItInsertionLine << '\n';<br>
}<br>
}<br>
<br>
@@ -572,7 +716,8 @@<br>
EmitCaretDiagnostic(LastLoc, Ranges, NumRanges, LastLoc.getManager(),<br>
Info.getCodeModificationHints(),<br>
Info.getNumCodeModificationHints(),<br>
- WordWrapped? WordWrapIndentation : 0);<br>
+ WordWrapped? WordWrapIndentation : 0,<br>
+ MessageLength);<br>
}<br>
<br>
OS.flush();<br>
<br>
Modified: cfe/trunk/test/Misc/message-length.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/message-length.c?rev=70597&r1=70596&r2=70597&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/message-length.c?rev=70597&r1=70596&r2=70597&view=diff</a><br>
<br>
==============================================================================<br>
--- cfe/trunk/test/Misc/message-length.c (original)<br>
+++ cfe/trunk/test/Misc/message-length.c Fri May 1 18:32:58 2009<br>
@@ -11,3 +11,12 @@<br>
<br>
int (*fp2)(int, float, short, float) = f;<br>
}<br>
+<br>
+void a_func_to_call(int, int, int);<br>
+<br>
+void a_very_long_line(int *ip, float *FloatPointer) {<br>
+ for (int ALongIndexName = 0; ALongIndexName < 100; ALongIndexName++) if (ip[ALongIndexName] == 17) a_func_to_call(ip == FloatPointer, ip[ALongIndexName], FloatPointer[ALongIndexName]);<br>
+<br>
+<br>
+ int array0[] = { [3] 3, 5, 7, 4, 2, 7, 6, 3, 4, 5, 6, 7, 8, 9, 12, 345, 14, 345, 789, 234, 678, 345, 123, 765, 234 };<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br>