<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On May 2, 2009, at 11:46 AM, Daniel Dunbar wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">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?</blockquote><div><br></div><div>I've done this... it looks okay to me. Et tu?</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug<br></div><div><br></div><br><blockquote type="cite"><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 <<a href="rdar://problem/6711348">rdar://problem/6711348</a>>. 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></blockquote></div><br></body></html>